Refactor (Registar) Managed Names #1433
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@shrugs Hey I like making this code less diffuse 👍 Reviewed and shared a few suggestions.
apps/ensindexer/src/plugins/registrars/ethnames/handlers/Ethnames_RegistrarController.ts
Outdated
Show resolved
Hide resolved
apps/ensindexer/src/plugins/ensv2/handlers/ensv1/NameWrapper.ts
Outdated
Show resolved
Hide resolved
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@shrugs Hey thanks for the updates 👍 Shared a few more suggestions
| * @dev Caches the result of namehash(name). | ||
| */ | ||
| export const getManagedName = (contract: AccountId) => { | ||
| export const getManagedName = (contract: AccountId): { name: Name; node: Node } => { |
There was a problem hiding this comment.
I'd like to see the ideas being advanced here fully integrated / harmonized with existing related ideas that I cited in #1433 (comment)
Specifically how I referenced the existing related logic in apps/ensindexer/src/lib/tokenscope/nft-issuers.ts
Here's how I understand it:
- We have a concept of a "SupportedNFTIssuer".
- Each "SupportedNFTIssuer" could then have:
- A "managed name" (which is a function of namespace)
- A "managed node" (a function of the "managed name")
- An
AccountIdidentifying the contract that actually issues the NFT (the "BaseRegistrar") (also a function of namespace) - Some set of additional
AccountIdidentifying all the contracts that are known to perform updates on this NFT Issuer, including the related "BaseRegistrar" from point 3 above as well as all "RegistrarControllers" associated with the "BaseRegistrar".
The idea in point 4 should then allow for a refined implementation of getManagedName where instead of the returned value being a tuple of name / node, it would return a SupportedNFTIssuer.
The getManagedName function should then also be renamed based on the ideas shared above.
Goal: I see high value in consolidating all our special logic and rules about specific NFT issuers together and not distributing it across many places. This should include the existing related work done in TokenScope.
There was a problem hiding this comment.
I understand that's your preference, but I'd like to avoid that level of abstraction and introduction of another conceptual layer: I think it harms understandability, and the usage of interpretTokenIdAs(LabelHash|NameHash) within the indexing handlers for that specific contract is a perfectly concise implementation of the same statements.
There was a problem hiding this comment.
@shrugs Sure open to that. What do you suggest for next steps then?
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@shrugs Sharing a thumbs up on this PR. Please take the lead to merge it in once ready. Thanks 👍
closes #1403