-
Notifications
You must be signed in to change notification settings - Fork 15
feat(ensapi): introduce Name Tokens API #1358
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: b5d0cdc The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
apps/ensindexer/src/plugins/registrars/ethnames/lib/registrar-helpers.ts
Outdated
Show resolved
Hide resolved
Also, fixes deserialization bug.
lightwalker-eth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tk-o Really happy to see this! Shared some suggestions and feedback. Appreciate your efforts to help make this API amazing 🫡
simplify name tokens api data model
Add `domainId` request param for Name Tokens API route
Update `NameTokensResponseError` variant types.
define request schema
Update code docs for `RegisteredNameTokens`
Introduce Name Token ownership concept
…r `AccountId` type.
lightwalker-eth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tk-o Hey great job! 🚀 Shared feedback please take the lead to merge this once ready 👍
| // Return 404 response with error code for unknown name context when | ||
| // the no name tokens were found for the domain ID associated with | ||
| // the requested name. | ||
| if (!registeredNameTokens) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe this should be a 404 error case.
Shouldn't it be a 200 success where we just return an empty array of name tokens?
|
|
||
| return useQuery({ | ||
| ...queryOptions, | ||
| refetchInterval: false, // no refetching - assume data is immutable until a full page refresh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we had a different strategy for this idea on other hooks we've implemented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lightwalker-eth, your previous feedback on that setting param:
In packages/ensnode-react/src/hooks/useNameTokens.ts:
+ refetchInterval: 60 * 1000, // 60 seconds - latest Name Tokens change infrequentlyNo, the results do not change frequently.
I would say it should be assumed to be effectively immutable until a full page refresh.
refetchInterval: false guarantees there query is immutable until the full page refresh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tk-o I was referring to ASSUME_IMMUTABLE_QUERY that we built. I assume that's what we want to use here?
| * c) Is no longer wrapped by the NameWrapper, and is also no longer | ||
| * minted by the BaseRegistrar (both tokens now burned by sending to | ||
| * the null address). | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's another edge case to consider which is defined here: https://namehash.slack.com/archives/C08BMPFUUDD/p1765288628066849
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cross-posting for clarity:
We may want to track independent
expiresAtvalues for eachNameToken. This can help us with goals including:
- The specific case we're discussing here, where we need the ability to represent that a particular
NameToken's registration lifecycle has expired, even though the token itself is still technically minted.- The additional case where the expiration time of a name in the NameWrapper can go out of sync with the expiration time of a name in the BaseRegistrar.
Replaced with `getNameWrapperAccounts` from ENSNode SDK.
Update code docs
lightwalker-eth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tk-o Looks great, I'm happy to merge this. Please take the lead to merge this and then we can follow up on comments in another PR 👍
| if (request.name !== undefined) { | ||
| const { name } = request; | ||
|
|
||
| // return 404 when the requested name was the ENS Root |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // return 404 when the requested name was the ENS Root | |
| // return 404 when the requested name was the ENS Root as otherwise | |
| // the call below to `getParentNameFQDN` will throw an error |
| * | ||
| * The latest Registration Lifecycle for a node referenced in | ||
| * `token.domainAsset.domainId`. | ||
| * The latest Registration Lifecycle for a node referenced in `domainId`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * The latest Registration Lifecycle for a node referenced in `domainId`. | |
| * The latest Registration Lifecycle for the node referenced in `domainId`. |
| */ | ||
| export const NameTokenOwnershipTypes = { | ||
| /** | ||
| * Name Token is owned by NameWrapper account. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * Name Token is owned by NameWrapper account. | |
| * The owner of this Name Token is a NameWrapper account. |
| /** | ||
| * Name Token is owned fully onchain. | ||
| * | ||
| * This ownership type can only apply to direct subnames of `.eth` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * This ownership type can only apply to direct subnames of `.eth` | |
| * Currently, this ownership type only applies to direct subnames of `.eth` |
|
|
||
| return useQuery({ | ||
| ...queryOptions, | ||
| refetchInterval: false, // no refetching - assume data is immutable until a full page refresh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tk-o I was referring to ASSUME_IMMUTABLE_QUERY that we built. I assume that's what we want to use here?
| /** | ||
| * Name Tokens for a registered name. | ||
| */ | ||
| export interface RegisteredNameTokens { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe then it's just a NameTokensResponse?
| * Find all Name Tokens for the registered domain by domain ID, | ||
| * accurate as of `accurateAsOf`. | ||
| * | ||
| * @returns {RegisteredNameTokens} if some tokens associated with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we action the follow-ups on this PR, Suggest we change all this "NameTokensNotIndexed" stuff to the more simple idea: "NameTokensUnknown"
| /** | ||
| * Name Tokens for the requested name. | ||
| */ | ||
| nameTokens: RegisteredNameToken[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appreciate that complexity. We should be able to improve this once we transition the expiresAt field into being a field for each NameToken in the response.
| * - Order of name tokens follows the order of onchain events that were | ||
| * indexed when a token was minted, or burned. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tk-o The more I think about it, the more I think we should just simplify this and say that the order of tokens returned in the response is undefined and not add any sort rules to it. There's no special need or goal for sort rules here.
Suggested review order: commit-by-commit.
This PR introduces new Name Tokens API route.
Example: `finalroll.eth` (minted, wrapped)
Example response:
Example: `tko.eth` (unwrapped, expired)
Example response:
Builds on top of #1357
TODOs
Pagination support? Might not be required, though. Saw up to two name token records while testing. Most of the names will have just a single associated name token record.Followup on
tokensfound for the requestedname, the response shoul be a 200 success where we just return an empty array of name tokens.RegisteredNameTokensentity that's returned by Name Tokens API.