Conversation
🛡️ Immunefi PR ReviewsThis pull request is not eligible for a PR Reviews review. Please contact Immunefi support. Reason: This PR can't be reviewed because no PR Reviews plan is configured for your organisation. Please ask your admin to set up a plan to enable reviews. |
Arachnid
left a comment
There was a problem hiding this comment.
We discussed this a while ago and you made a good argument that having a (registry, token id) pair was a better option than a name. Have you changed your mind?
|
Yeah, there's a slack discussion (where I'm arguing with myself 🤣️) and #209 is the other implementation. They're basically equivalent and only differ in their properties: #209 #217 /// @dev `name` is canonical if it's ancestors are canonical and share parential suffixes:
/// eg. Canonical not-Canonical not-Canonical
/// ========= ============= =============
/// abc.sub.test.eth abc.eth a.b.test.eth
/// sub.test.eth > xyz < b.test.eth
/// test.eth <root> > nick.eth <
/// eth eth
/// <root> <root>Ignoring storage, it's ultimately the choice between:
|
|
Using #209 |
IRegistrygetCanonicalName() returns (name)event CanonicalNameUpdatedIStandardRegistrysetCanonicalName(name)PermissionedRegistryROLE_SET_CANONICAL_NAMEsetCanonicalName()andgetCanonicalName()LibRegistryfindCanonicalRegistry(root, name, offset) returns (registry)UniversalResolverV2isCanonical(name) returns (bool)— unsure if this is the right function to exposedoes anamehash() == namehash()check that could be a keccak check⭐️ Personally, I like the #209 approach (as immediate access to the parent seems useful and it avoids someone using an non-canonical name) but this matches the original design that was discussed. It's also cleaner when you move things around, since only 1 canonical reference needs changed, instead of every descendant.