'Generate ENS referral link' form refinement - #60
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.
@Y3drk Hey great to see these updates. The logic all looks to produce the correct outputs 👍 So please feel welcome to take the lead to merge this PR when ready. The feedback I shared here is all focused on optimizing the way the code is written 👍
| import { cn } from "@/utils/tailwindClassConcatenation.ts"; | ||
| import { Link2 as LinkIcon, RefreshCw as RefreshIcon } from "lucide-react"; | ||
| import React, { type FormEvent, useEffect, useState } from "react"; | ||
| import { ENSNodeClient, type Name, type ResolveRecordsResponse } from "@ensnode/ensnode-sdk"; |
There was a problem hiding this comment.
I note here how you are integrating with the ensnode-sdk package.
Is there a special reason why you chose to integrate at that level rather than using the ensnode-react package?
There was a problem hiding this comment.
There are a few reasons why I wanted to go a level lower here:
- Due to how
astro islandswork, each one of these would need to have a separate<ENSNodeProvider />. While it's not an issue currently, I'd prefer to limit their number as much as possible, especially since we will have to use it 100% forlatest indexed registrations. It might or might not turn out not to be a performance issue, but it definitely makes the code more obscure and potentially harder to debug. - In this form, the resolution only runs once per scenario, when the user clicks "Generate", hence there is no need for caching or refetching the tanstack query inside the ensnode-react provides. Ofc it's still possible to set up the
useRecordshook for that, but it feels a bit like using a sledgehammer to crack a nut... - ... and due to multiple validation steps performed in the submit function, the
isLoadingfeature doesn't bring any immediate benefits either, as the "loading" state is set way before the actual resolution starts, and the end of it doesn't mean the loading ends either.
All in all, I just feel like for this particular form submit, using the React wrapper is an overkill.
Both approaches are possible, but for this case, I'd prefer to keep using the ensnode-sdk directly.
There was a problem hiding this comment.
@Y3drk Ok cool. Really appreciate your message here on why. Thanks
|
|
||
| const lookupENSName = async (name: Name) => { | ||
| const lookupPromise = client.resolveRecords(name, { | ||
| addresses: [60], // ETH CoinType |
There was a problem hiding this comment.
Suggest reviewing logic we implemented in ENSAdmin for making use of the resolution API:
You can review all places in our codebase that are already making use of useRecords for inspiration, including: https://github.com/namehash/ensnode/blob/57bd01dfb41b8b95f0aea8b8942eb1f68311632c/apps/ensadmin/src/app/inspect/records/page.tsx#L44-L60
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@Y3drk Hey thanks for your updates. Reviewed and shared feedback 👍
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@Y3drk Hey looks good, thanks 👍 I pushed a few small refinements. There's still 1 key remaining open question for you. Please take the lead to merge this PR as soon as you're ready. Thanks 👍
2 participants
Implements a new, extended version of the
Generate your ENS Referral Program linkform.More details available in this Slack thread.
Some dependency vulnerabilities are taken care of in PR #58, and some version bumps are a part of this PR as well.