feat(namekit-react): Identity #439
Conversation
notrab
requested review from
djstrong,
Carbon225,
lightwalker-eth,
FrancoAguzzi,
BanaSeba and
edulennert
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| @@ -43,6 +43,7 @@ | |||
| "@headlessui/react": "1.7.17", | |||
| "@namehash/ens-utils": "workspace:*", | |||
| "@namehash/ens-webfont": "workspace:*", | |||
| "@namehash/nameguard": "workspace:*", | |||
There was a problem hiding this comment.
Your suggestion to make this a peer dependency sounds good 👍
There was a problem hiding this comment.
FYI: This hasn't been changed yet.
|
|
||
| if (loading) return <div>Loading...</div>; | ||
| if (error) return <div>Error: {error}</div>; | ||
| if (!data) return null; |
There was a problem hiding this comment.
Suggest a few updates here:
- The
Rootand all of its children should always be rendered, even if data is still loading or failed to load. The responsibility for handling these cases should be pushed down into child components. Ex:Avatarshould know what to do when it is loading or when there was a loading error, etc.. - ... continuing on from above: Suggest updating the interface that is returned by
useIdentityso that all of the following are true:- Independent of any loading state, it is always guaranteed to be able to get the following values:
addressandnetwork(these props should never be undefined). - There's some distinct field(s) representing the loading state / request error state for the identity.
- We shouldn't have the returned value
... extends SecurePrimaryNameResult(because this value might still be loading, etc..). Therefore we should have this as an optional property of the returned value that is undefined unless the loading state field is some value representing it has been successfully loaded.
- Independent of any loading state, it is always guaranteed to be able to get the following values:
There was a problem hiding this comment.
I think I have addressed most of this in a recent update. I need to double check 2.2
notrab
force-pushed
the
account-pill-playground
branch
from
5594864 to
ef9dbc5
Compare
There was a problem hiding this comment.
@notrab Hey, great to see the progress you've made here. Shared some feedback for our overall strategy on next steps 👍 Appreciate your advice!
| } | ||
|
|
||
| return ( | ||
| <a |
There was a problem hiding this comment.
Appreciate your advice. Should we use our <Link for this instead of <a?
There was a problem hiding this comment.
In theory, yes. But I want to make more progress on the core implementation before moving to UI.
| className={`namekit-ens-profile-link ${className}`} | ||
| {...props} | ||
| > | ||
| <ENSLogo /> |
There was a problem hiding this comment.
What do you think about moving ideas such as:
<ENSLogo /> and the subsequent <span ...> aren't hard coded into
ENSProfileLink but we just put any children that might be defined there instead?
This seems more composable? Ex: It's nice to be able to customize that profile link however you want.
The main purpose of this ENSProfileLink as I see it is to manage the logic for how you generate the link for the profile. But the click target ideally isn't hardcoded.
Appreciate your advice.
| network?: Network; | ||
| className?: string; | ||
| children: ReactNode; | ||
| returnNameGuardReport?: boolean; |
There was a problem hiding this comment.
@notrab Hey, suggest extending this idea. When someone defines an Identity Root, suggest that they also identify what lookups they want to make for that identity.
For example, suggest we support optional lookups of each of the following:
- NameGuardReport.
- EFP
- Stats (https://docs.ethfollow.xyz/api/users/stats/ -- you've already implemented this in a way, just suggesting it is restructured to be optional).
- Common Followers (https://docs.ethfollow.xyz/api/users/commonfollowers/)
- Is current user following this identity? (https://docs.ethfollow.xyz/api/users/followerstate/)
- ENS Profile, for example, social media handles, etc.. To save some time, we should take advantage of existing services for this. Suggest we use this service for now: https://enstate.rs/docs#tag/single-profile/GET/u/{name_or_address}
A few other important suggestions here:
- For items 2 and 3 listed under EFP above, we also need to know the address of the current user (as opposed to the address of the identity we want to show data about). Please see my separate comment where I suggest we add wagmi as a dependency. If we do this, we can take advantage of the https://wagmi.sh/react/api/hooks/useAccount hook for this purpose. Then, based on the value returned by the
useAccounthook, we could have some intelligent logic inside the EFP related components: If the current user is known, we also work to load the items 2 and 3 listed under EFP above. If we don't know who the current user is, then we don't load items 2 and 3 listed above, since that context only makes sense in relation to knowing who the current user is. - Strongly suggest we don't try to do all this data fusion ourselves between all these different API calls on our frontend. Instead, suggest we move this data fusion API calls inside our own backend for a number of reasons. Let's keep it maximally simple for now and just do the most basic implementation of this API inside the backend of one of our existing open source Next.js apps that are in our monorepo (such as the NameGuard app). We can do a more advanced version of this API in a separate future PR that would create a new app for api.namekit.io where we could move this backend logic in the future).
- Therefore, the API call to NameGuard would move to our backend and not happen in the frontend here. Please ask me if any questions. I'm trying to simplify the frontend so that it only needs to make 1 API call per Identity.
- One theoretical downside to moving all this work to the backend is increased latency for showing partial results. Ex: If data needs to be loaded via 4 external API calls, and the backend waits for all 4 API responses before returning its own response, the frontend might have to wait longer if 1 of those 4 API responses is meaningfully slower than others. However, I don't suggest we worry about that for now. If we identity this as an issue / opportunity for the future we can pick from a number of techniques where our backend API can stream data to the frontend as it loads.
- It seems to me it would be a nice DX to make it so the Identity context / hook would include info on which items were requested to be loaded from the API. In other words, it seems nice if these components could separately identify the case of we never requested to load the ENS Profile vs there being no ENS profile for a given identity. Does that make sense?
There was a problem hiding this comment.
@lightwalker-eth this is fantastic feedback, and the ideas are awesome.
I do prefer a DX whereby there's less configuration. So if you use the Identity.Followers then it knows to fetch followers (like it does now). The only thing not following this is nameGuardReport, it could be messy but I think we could export another component that is Identity.NameGuardReport and if that's used at all, the initial query returns the report too. If that makes sense.
However, to be more explicit with the "options" available as you suggested, we could do something like this:
interface IdentityLookupOptions {
nameGuardReport?: boolean;
efpStats?: boolean;
efpCommonFollowers?: boolean;
efpFollowState?: boolean;
ensProfile?: boolean;
}Which means we can do something like this:
<Identity.Root
address="0x..."
network="mainnet"
lookupOptions={{
nameGuardReport: true,
efpStats: true,
ensProfile: true,
}}
>
<Identity.Avatar />
<Identity.Name />
<Identity.Address />
<Identity.NameGuardShield />
<Identity.ProfileLink>View Profile</Identity.ProfileLink>
<Identity.Followers />
</Identity.Root>|
|
||
| const nameguard = createClient({ network }); | ||
|
|
||
| const result = await nameguard.getSecurePrimaryName(address, { |
There was a problem hiding this comment.
Suggest that we add ReactQuery as a dependency / peer dependency for NameKit React.
The web3 community has generally adopted ReactQuery as an unofficial standard. There's a ton of various libraries and packages already using it or built on it.
For example, strongly suggest having a look at the following:
- wagmi. https://wagmi.sh/ The same team that built wagmi built viem https://viem.sh/. Pretty much everyone is using both libraries in their web3 apps. viem is a more low-level library that's suitable for use on a backend or frontend. wagmi is a frontend library that wraps viem. wagmi uses ReactQuery for all their network requests.
- onchainkit. https://onchainkit.xyz/ This is built as a layer on top of wagmi.
- rainbowkit. https://www.rainbowkit.com/ This is also built as a layer on top of wagmi.
- Etc..
In fact, suggest that we not only add ReactQuery as a dependency, but we also add wagmi as a dependency. We shouldn't try to reinvent the whole universe. Pretty much everyone who might use NameKit React is also going to be using wagmi, so we better make it convenient for everything to work together.
|
|
||
| return ( | ||
| <a | ||
| href={`https://app.ens.domains/${identityData.display_name}`} |
There was a problem hiding this comment.
We need some strategy for people who use NameKit React to customize the link target for viewing a detailed profile. This needs to support a variety of cases, including calling a function to a traditional href, customizing opening in a new tab or not, etc..
Additionally, the way this link is built may vary. For example, in some apps the link to a profile is based on an address. In other apps, the link is based on an ENS Name (where possible) and only uses a link with an address as a fallback if the address has no primary name.
For example, consider an app such as EFP. When someone clicks on a profile link on their app, they don't want it to open to the official ENS App. They want it to open to the profile page on their own app.
I worked on some ideas for how a goal like this in the following PR. Open to other suggested strategies for achieving this dynamic logic: Check the logic around the file packages/nameguard-react/src/utils/openreport.ts it might give some inspiration? https://github.com/namehash/namekit/pull/282/files#diff-4b7ca1b4f908b4cf07782ef287606daa30f6c17f8181e6d7a3ab82eb09137dbd
There was a problem hiding this comment.
@lightwalker-eth this is what I have in mind that I would like to experiment this week...
1. Standard implementation
This shows the default link:
<Identity.Root address={address}>
<Identity.ENSProfileLink />
</Identity.Root>2. Custom NameKit Config
const efpConfig = {
profileLinks: {
getProfileTarget: ({ address, ensName }) =>
`/profile/${ensName || address}`,
openInNewTab: false,
// Optionally use Next.js Link or other routing component
LinkComponent: NextLink,
}
};
<NameKitConfigProvider config={efpConfig}>
<Identity.Root address={address}>
<Identity.ENSProfileLink />
</Identity.Root>
</NameKitConfigProvider>Optionally, a click handler similar to what you suggested:
const handlerConfig = {
profileLinks: {
getProfileTarget: ({ address, ensName }) =>
(e: React.MouseEvent) => {
e.preventDefault();
// Custom handling logic
openProfileModal(address, ensName);
}
}
};This allows us to move towards the "React Query" context provider for data too.
There was a problem hiding this comment.
@notrab Hey, thanks for putting this together and sharing it 🙌 🚀
A few suggestions:
getProfileTargetam I correct to understand this is supposed to return a string representing a URL? If so, suggest renaming this togetProfileURL.- I also see your separate click handler example, but confused how everything fits together here. Because here the
getProfileTargetdoesn't return a string, it returns a function that takes aReact.MouseEventparam.
- I also see your separate click handler example, but confused how everything fits together here. Because here the
openInNewTabandLinkComponentI like the general direction you're working towards here for supporting more customizability. But maybe we use a different strategy? What do you think of modifying the approach there to instead have a function calledgetProfileLinkthat takes in aurl(as might be generated bygetProfileURLand then returns a DOM element tree for turning thaturlinto the HTML for a link.- This strategy not only would make customizing link targets / link components no problem, but I believe it should also give a solution for the "click handler" case.
- Maybe instead of
getProfileLinktaking as input aurlas a param, it should instead take the same param type asgetProfileTarget/getProfileURL. Then, ifgetProfileLinkwants to build a link with a URL in it, it can callgetProfileURLitself to do that. Alternatively, ifgetProfileLinkwants to do some javascript-based "click handler" it can then more easily define that logic if the param passed togetProfileLinkwere an address or an ensName.
- For now, maybe we exclusively build support for getting the profiles using address, not using ensName. I'm worried supporting both will slow us down right now. It's definitely nice to ultimately add support for both in the future, but suggest we come back to that later. For example, in the future we might define some special interface for what gets passed into
Identity.Rootwhere it is either a name or an address and not both. This same interface could then be passed around to other downstream components such as the callback functions defined in the config, etc.. But suggest we keep things more simple for now and only focus on identities that are anchored in an address and not an ensName. - Suggest that we define some default handlers for the profile link configs. The default should result in opening to view the profile of the provided address on the ens.domains website in a new tab. Ex: https://app.ens.domains/0xf81bc66316A3f2A60Adc258F97F61dFcBdd23Bb1
- It seems nice to make it so that
Identity.ENSProfileLinkjust defines a link, not the anchor elements for the link. As I understand we should allow any children to be passed intoIdentity.ENSProfileLinkwhich would then become the anchor for the link. As I understand, just doing<Identity.ENSProfileLink />should be similar to doing<a />in that there shouldn't be anything for a user to see or interact with until you define the children beneath that. - Whatever interface we decide on for setting the config for profile links, it might be nice to allow a custom config for a specific
Identity.ENSProfileLinkinstance to be passed in. By default people wouldn't do that, so then it would fallback to the default config set inNameKitConfigProvider. - Perhaps good to rename
Identity.ENSProfileLinkto justIdentity.ProfileLink. I like making "ENS" implied rather than explicit. For example, usenameinstead ofEnsName, etc.. If we don't do this we'll have "ENS" too much all over our code.
Appreciate your advice 👍
There was a problem hiding this comment.
@lightwalker-eth I've applied most of this feedback in a new commit. I agree with it all.
I also updated the stories to match the new requirements too. I hope I didn't miss anything, it took some refactoring/rethinking to make it all work better 🙏🏻
There was a problem hiding this comment.
@lightwalker-eth I made some changes based on what you said about the idea of a class for the ProfileLinkGenerator.
export class ProfileLinkGenerator {
private baseURL: string;
private name: string;
constructor(name: string, baseURL: string) {
this.name = name;
this.baseURL = baseURL;
}
getName(): string {
return this.name;
}
getProfileURL(address: string): string {
return `${this.baseURL}${address}`;
}
}This is basic for now, but acts as a way to set a base URL, and we could even export some default ones, such as:
export const ENSProfileLink = new ProfileLinkGenerator(
"ENS",
"https://app.ens.domains/",
);Then this can be used wherever:
interface ProfileLinkProps {
config?: ProfileLinkGenerator;
className?: string;
children?: React.ReactNode;
onClick?: (e: React.MouseEvent) => void;
}
const ProfileLink: React.FC<ProfileLinkProps> = ({ config, children, onClick }) => {
const identity = useIdentity();
const nameKitConfig = useNameKitConfig();
const linkConfig = config || nameKitConfig.profileLinks?.[0] || DEFAULT_PROFILE_LINKS[0];
if (!identity) {
console.warn("ProfileLink used outside of Identity context");
return null;
}
const url = linkConfig.getProfileURL(identity.address);
return (
<a
href={url}
target="_blank"
rel="noopener noreferrer"
className="namekit-profile-link"
onClick={onClick}
>
{children || linkConfig.getName()}
</a>
);
};This also allows ProfileLinks to be controlled individually, or part of the context provider down. Giving more flexibility.
| profileLinks: { | ||
| getProfileURL: (address: string) => `/profiles/${address}`, | ||
| getProfileLink: (address: string, children: React.ReactNode) => ( | ||
| <a href={`/profiles/${address}`}>{children}</a> |
There was a problem hiding this comment.
Seems nice to implement this example such that it calls getProfileURL.
| <Identity.Avatar /> | ||
| <Identity.Name /> | ||
| <Identity.ProfileLink> | ||
| <button>View Profile</button> |
There was a problem hiding this comment.
Any special reason not to use our Button?
There was a problem hiding this comment.
As I mentioned before, I'm avoiding the UI side of things until we nail the DX. Swapping out components is easy, but if we need to make changes to the UI components, it could cause conflicts so I've opted not to focus on that yet.
| @@ -21,7 +21,7 @@ export default meta; | |||
|
|
|||
| type Story = StoryObj<typeof Identity.Root>; | |||
|
|
|||
| const IdentityCard: React.FC<{ | |||
| const DefaultIdentityCard: React.FC<{ | |||
| address: string; | |||
There was a problem hiding this comment.
Please check all places in the code where you have an address param. These should all use the Address type defined in viem.
| profileLinks: { | ||
| getProfileURL: (address: string) => `/profiles/${address}`, | ||
| getProfileLink: (address: string, children: React.ReactNode) => ( | ||
| <a href={`/profiles/${address}`}>{children}</a> |
There was a problem hiding this comment.
Any special reason not to use our <Link in this example?
| } | ||
|
|
||
| interface NameKitConfig { | ||
| profileLinks?: ProfileLinkConfig; |
There was a problem hiding this comment.
Suggest we make it so that this field is always defined. If no custom generator was provided, then we should make it equal to the default generator we define.
| </a> | ||
| ); | ||
| const config = instanceConfig || | ||
| globalConfig.profileLinks || { |
There was a problem hiding this comment.
Suggest extracting some of the ideas here out into a named / exported class that provides a default implementation of ProfileLinkGenerator / ProfileLinkConfig.
One thing that can be nice about this: Someone could create a subclass of the default ProfileLinkGenerator implementation and only override the getProfileURL function in it and all the other defaults can continue to work.
| export interface ProfileLinkConfig { | ||
| getProfileURL: (address: string) => string; | ||
| getProfileLink: (address: string, children: ReactNode) => JSX.Element; | ||
| } |
There was a problem hiding this comment.
I shared some other comments in this PR with the idea of having a default fallback implementation of the profile link anchor. Ex: An ENS Logo, etc..
It seems to me that one nice way to do this would be to define another function in this interface, for example: getProfileLinkAnchor that takes no params and returns a JSX.Element. We could have a default implementation that provides an anchor that is an ENS Logo, for example.
Appreciate your advice / suggestions 👍
|
|
||
| const CustomAppIdentityCard: React.FC<{ address: string }> = ({ address }) => ( | ||
| <NameKitProvider config={customAppConfig}> | ||
| <Identity.Root address={address}> |
There was a problem hiding this comment.
Suggest we make a few updates here to put our "Identity" components on a stronger foundation. Specifically: an "identity reference" is more than just an address. It is a combination of a chain and an address.
- Suggest defining an interface for
ImplicitIdentityReferencethat combines both an address and a chain, but where the chain param is optional. The purpose of this interface is to optimize DX for the average case where all addresses within an app are on the same chain. It should be noted how this isn't always the case though, and increasingly won't be in the future the way things are headed. - Suggest defining an interface for
IdentityReferencethat extendsImplicitIdentityReferencebut makes the chain param required. - Suggest defining a little utility function that can be used within the context of a NameKitProvider that automatically converts any
ImplicitIdentityReferenceto an explicitIdentityReferenceusing the configured chain as the default. - There's a bunch of functions here that currently only take an "address" param. These should change so that they instead take a param of type
IdentityReferenceorImplicitIdentityReference. Inside the implementation of these functions, we can call the utility function described above that takes either anIdentityReferenceorImplicitIdentityReferenceas a param and always returns an explicitIdentityReferencefor use in all our internal logic. - Functions such as
getProfileURLandgetProfileLinkshould take anIdentityReferenceas a param. The default implementation of these functions should then consider the chain parameter and not only the address. For example. If the chain param is mainnet, then our existing strategy is good. If the chain param is anything else (such as sepolia, etc..), then there isn't an existing profile service we can easily link to for this. So instead suggest making our default implementation of all the profile link generator functions trigger some alert box or something like that for the case that the chain is anything but mainnet. Once we build the backend APIs into NameKit for generating profile data, we can come back to this and implement some improved default behaviour for showing profiles on chains other than mainnet.
Here's an acceptance criteria for everything described above: If I use our Identity components to show the identity for a chain other than mainnet (such as sepolia) I shouldn't get a profile link to view the profile of that address on mainnet as that wouldn't actually be the correct profile / identity.
Please ask me if any questions 👍
There was a problem hiding this comment.
@notrab Hey thanks for updates. Reviewed and shared feedback.
| "https://app.ens.domains/", | ||
| ); | ||
|
|
||
| const DEFAULT_PROFILE_LINKS = [ENSProfileLink]; |
There was a problem hiding this comment.
Interesting. Can you help me understand the goal of having an array here?
| interface NameKitConfig { | ||
| profileLinks?: ProfileLinkConfig; | ||
| profileLinks?: ProfileLinkGenerator[]; |
There was a problem hiding this comment.
Similar to the feedback above. Can you help me understand the goal for representing this as an array? In my thinking there should be exactly one, not 0 or many.
| profileLinks?: ProfileLinkGenerator[]; | |
| profileLinkGenerator: ProfileLinkGenerator; |
| getProfileURL: (address: string) => string; | ||
| getProfileLink: (address: string, children: ReactNode) => JSX.Element; | ||
| export class ProfileLinkGenerator { | ||
| private baseURL: string; |
There was a problem hiding this comment.
I don't think this should have a "baseURL". This assumes too much about how the URLs will be constructed, especially when considering how they should consider both chain and address.
| this.baseURL = baseURL; | ||
| } | ||
|
|
||
| getName(): string { |
There was a problem hiding this comment.
Can you help me understand the goal for getName? It will help a lot to document ideas in comments.
| ); | ||
| }; | ||
|
|
||
| interface ProfileLinksProps { |
There was a problem hiding this comment.
Let's have a chat to help align on goals here. I think we're drifting away from solving the problems that people are actually looking to solve.
This PR adds a preview for the mechanics of the
<Identity />component. You can see a preview of it here:CleanShot.2024-10-15.at.18.42.13.mp4
Keep in mind that this is a technical preview, and no styling has been implemented yet, and I won't spend time on the styles until we know we are fetching all the data we want, and the DSL is nice. There is also no followers fetched yet.
One thing I dislike about PR is it now adds as a dependency
@namehash/nameguard. Instead, I would prefer this to be a peer dependency, and anyone wanting to use this component should install@namehash/nameguardAND@namehash/namekit-react.