-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Missing domain error #70
Comments
Looking at [the related transaction logs], the very first event in the log is causing the runtime error on the ensnode/src/handlers/Registrar.ts Lines 146 to 151 in 64054e1
Debugging contextLabelhash & NamehashA registration entity uses the labelhash value as its primary key. I've checked the labelhash and namehash values at https://tools.ens.xyz/check/quantamm.eth Event log
What do you think about that @shrugs & @lightwalker-eth? |
Now I think we might have a cross-chain registration conflict. The We have the same label registered in two different parent domains:
|
@tk-o Fantastic debugging! I believe this issue might be related to the goals I was hoping to advance in #44 We need to separate out the following ideas:
In the BaseRegistrarController for .eth names, the labelhash of the direct subname of .eth is used as the NFT token id. In the NameWrapper (for all ENS names), the namehash (of the entire name) is used as the NFT token id. The labelhash of a subname is not safe for use as a unique identifier across all of ENS. Only the namehash of the entire name is. |
Please also note how the ENS Protocol says nothing at all about NFTs or NFT token ids. The implementation details of NFTs and NFT token ids are separate ideas from ENS itself. |
aha! so because perhaps a simple solution, though it isn't very pretty, is to just pass another argument to const registrationId = `${keyPrefix}${label}` but we should add a comment about this, and note in v2.md that we want to correctly key these things in the future (likely by namehash). wdyt? |
@shrugs Could you please verify that the subgraph only tracks registrations under .eth and not at all within the NameWrapper (which goes beyond .eth, and also uses a different token id strategy for .eth NFTs). Could you please help me better understand the background / goals of what we need a function like |
I've checked the stats for the -- labelhash(`www`)=`0x3177317affd6342ed2401ccea23053d41b86d0914b5b1bee0faa1efcb7221a61`
SELECT * FROM "25ad4bb7-8697-4788-bf11-5253ca96026c".registrations
WHERE "id" = '0x3177317affd6342ed2401ccea23053d41b86d0914b5b1bee0faa1efcb7221a61';
SELECT * FROM "25ad4bb7-8697-4788-bf11-5253ca96026c".domains
WHERE labelhash = '0x3177317affd6342ed2401ccea23053d41b86d0914b5b1bee0faa1efcb7221a61'; There's just one record in |
This issue exists on the Subgraph API as well: check out this query. In query results, only one |
I mean that because the wdym different tokenId strategy? ETH Registrar and NameWrapper both use labelhash encoded as unit256, right? the what tko has discovered is that there is another registrar-specific piece of context that these otherwise-generic handlers need, and it's to further scope the Registration record's |
yes, which is by design. the Registration record only tracks .eth Registrar events, and the others are subdomains of those domains. only one of these domains is a direct subdomain of .eth (it's |
the .eth plugin, when run alone, will never run into this issue because labelhash is a perfectly good unique id for a Registration record (again because the assumption is that a Registration is specifically representing an action taken in the ETHRegistrar), as long as one does not index any other Registrars. we have discovered this issue because that's exactly what we're doing. like mentioned, the solution is a complete refactor of the schema, to better represent the actual on-chain state and specifics of the ENS protocol. the subgraph encodes many assumptions, and this is one of them. |
Thanks guys. Several points:
@shrugs I mean that different contracts can use different strategies for deciding on the relationship between a name and a tokenId. The BaseRegistrar uses the labelhash of the direct subname. The NameWrapper uses the namehash of the full name. Etc. And thanks for the details about For Subgraph compatibility, it seems we should try to continue using exactly the same Registration id strategy within the .eth BaseRegistrar. However, for our other plugins, couldn't we just do something simple such as prefix those registrations by a fixed constant such as "namehash:{namehash}" or something like that? In other words, we can take advantage of how the .eth BaseRegistrar strategy for Registration ids is just labelhash of the direct subname's label. Therefore, as I understand, in our other plugins, we just need to define a different id issuance strategy that is guaranteed not to conflict. @shrugs Agreed that we want to refactor our schema a lot for V2+, but for V1 as I understand we should continue to implement creative solutions as needed to achieve our multi-plugin goals while still conforming to the ENS Subgraph schema. If your understanding is different please let me know. Cheers. |
Thanks for the extra context, @shrugs. Perhaps, to retain subgraph compatibility, but solve the cross-chain labelhash issue, we could add new subname helpers: export function ownedNameToTokenIdPrefix(ownedName: `${string}eth`): string {
return ownedName.endsWith("eth") ? ownedName.slice(-3) : "";
}
export function prefixedTokenIdToLabel(tokenId: bigint, prefix: string): `0x${string}` {
const tokenIdHex = toHex(tokenId, { size: 32 });
if (!prefix) {
return tokenIdHex;
}
return concat([toHex(prefix, { size: 32 }), tokenIdHex]);
} and then use the available const tokenIdToLabel = (tokenId: bigint) =>
prefixedTokenIdToLabel(tokenId, ownedNameToTokenIdPrefix(ownedName)); |
ah, i see, thanks for the specificity. will include this note in #44 correct, a different id issuance strategy is needed for the non-.eth plugins. i proposed a very simple one, with a dumb prefix to the labelhash, as a stopgap for our v1. the @tk-o too complicated, in my opinion. since all we need to do is scope Registration ids by the Registrar that is creating them, i really do like the simple prefix. in fact, we don't need to pass a new parameter at all, we already have // in handlers/Registrar.ts
// scope non-.eth plugin Registration records by ownedName, to avoid labelhash uniqueness conflicts
// TODO: in v2, key Registrations by namehash
const makeRegistrationId(ownedName: string, labelhash: Hex) => ownedName === '.eth'? labelhash : `${ownedName}:${labelhash}`; and then use that helper to construct ever Registration id in that file. |
not sure where you're seeing the need for a tokenIdToLabel update in the context of this issue. the registrars will continue to issue uint256 of labelhash. and we then prefix it for the record's id. don't see a situation in which we need to unpack a prefixed id |
Just went with my gut feeling, sir 🙃 I guess I wanted to avoid refactoring by just replacing the const makeRegistrationId(ownedName: string, labelhash: Hex) => ownedName === 'eth'? labelhash : `${ownedName}:${labelhash}`; |
@shrugs I like your idea for |
Transaction details: https://etherscan.io/tx/0x109e7530e5c1d5d4e45c2623df553901d470117a31704e5793efd3152b5c89b4
Full error message
The text was updated successfully, but these errors were encountered: