feat: new OnchainName component #49

alvaroraminelli · 2024-01-31T00:52:56Z

What changed? Why?

Adding the first OnchainName component of our Identity Kit 👤

The OnchainName component primarily focuses on showcasing ENS names for given Ethereum addresses, and defaults to displaying a sliced version of the address when an ENS name isn't available.

import { OnchainName } from '@coinbase/onchainkit';

<OnchainName address="0x1234567890abcdef1234567890abcdef12345678" sliced={false} />;

Notes to reviewers

How has it been tested?

Unit tests only.

Zizzamia · 2024-01-31T06:46:24Z

README.md

@@ -213,6 +213,29 @@ type FrameMetadataResponse = {
 <br />
 <br />

+## OnchainName 📛


This new section will be called Identity Kit, which will have a callection of components and utilities around Onchain Identity.

README.md

Zizzamia · 2024-02-01T02:27:42Z

README.md

+<OnchainName address="0x1234567890abcdef1234567890abcdef12345678" sliced={false} />;
+```
+
+**@Param**


Oh, I think we can use types going forward to better describe what's going on. Check the latest README.

soon we might have autogen docs that will be based on type correct?

Maybe... There is a difference between type information, and how to teach how to use something.

Vitepress will automate docs creation in some capacity, but the ART of teaching how to use a function is still a manual work.

README.md

Zizzamia · 2024-02-01T02:30:24Z

src/components/OnchainName.tsx

+import type { Address } from 'viem';
+
+type OnchainNameProps = {
+  address?: Address;


I think the address should be mandatory

If OnchainName accepts undefined address

import { useAccount } from 'wagmi'; import { OnchainName } from '@/components/OnchainName'; function App() { // may be undefined const { address } = useAccount(); // Will return empty span if address is undefined return <OnchainName address={address} /> }

If OnchainName rejects undefined address

import { useAccount } from 'wagmi'; import { OnchainName } from '@/components/OnchainName'; function App() { // may be undefined const { address } = useAccount(); if (!address) { return null; } // Will cause type error if we do not check address first return <OnchainName address={address} /> }

Looking at likely example use case, it feels less cumbersome to allow address to be undefined. Thoughts?

agreed with @cnasc. vote to accept to make less cumbersome for consumers.

You just made my point, you allow user to shot themself if the address is not there. And than they end up in a broken state.

vote to accept to make less cumbersome for consumers.

The consumer will see an empty name, that's not a good experience.

@cnasc @alvaroraminelli what am I missing? If you want an OnchainName, you need a an address. Simple. Why are we allowing folks to say, I have not an address today. It's a bit saying, I go to a coffee shop I ask for coffee, I don't have money, and still expect an empty cup of coffee because I have no money. Well if you don't have money I want someone to tell me, sorry no coffee until you have money.

Right?

Zizzamia · 2024-02-01T02:31:26Z

src/components/OnchainName.tsx

+export function OnchainName({ address, className, sliced = true, props }: OnchainNameProps) {
+  const { ensName } = useEnsName(address);
+
+  const normilizedAddress = useMemo(() => {


Add a comment here why we have useMemo

Also should we avoid have CPU execution for the normilizedAddress if in case of ensName we will use that anyway?

Suggested change

const normilizedAddress = useMemo(() => {

const normalizedAddress = useMemo(() => {

Also, I think the idea here is that the input address will display until ens name resolves. I don't think that's unreasonable, but maybe we'd prefer an explicit loading state?

Great call. Yes, we need loading state here. Wondering how the UX would look like when you have a list of users in the UX, will things shift and change like crazy. User might opt-in to display loading state.

Need some UX thinking here.

Zizzamia · 2024-02-01T02:35:14Z

src/store/client.ts

+import { createPublicClient, http } from 'viem';
+import { mainnet } from 'viem/chains';
+
+export const publicClient: ReturnType<typeof createPublicClient> = createPublicClient({


I think client should go in src/network or more a location where we handle Data Layer stuff. cc @robpolak

Zizzamia · 2024-02-01T02:35:50Z

src/store/inMemoryStorageService.ts

+
+// InMemoryStorageService: A service to store data in JavaScript memory.
+// It's a simple key-value storage using a Map. The data is stored as long as the page is not reloaded.
+export class InMemoryStorageService implements StorageInterface {


Let's convert this to functional at some point. Let's keep OnchainKit class free.

Do we see this as a frontend-only use case? If so, it might be best to expose this as a context

I see this evolving towards sharing in context as well. We might want build a light version of react-query for actions.

I see this evolving towards sharing in context as well.

React Context are a slippery slop, where folks think everything should be a context. Library should have like 90% of their state being just variable shared between functions. And for very very small exential stuff you can have Context that optimizes the Hooks story.

So before we build any React Context, it's important to ask the question, on where is the benefit for the end application. If it's just a nice to have for us as developer, than we don't need that. But again, for Hooks Rendering optimization, that we can talk about it.

Zizzamia · 2024-02-01T02:40:36Z

Before we merge, let's have also a quick review from @cnasc, @robpolak, @wespickett and @Sneh1999 on this epic first Identity component.

cnasc · 2024-02-01T13:00:00Z

src/components/OnchainName.test.tsx

+  it('displays sliced address when ENS name is not available and sliced is true as default', () => {
+    (useEnsName as jest.Mock).mockReturnValue({ ensName: null });
+    (getSlicedAddress as jest.Mock).mockImplementation(
+      (addr) => addr.slice(0, 6) + '...' + addr.slice(-4),


Does this need to be mocked? I notice that the implementation here is slightly different from the one in the real function. Is that significant?

tried the approach below but it is not working. fallback to mocking the entire function.

import * as addressUtils from '../core/address'; beforeEach(() => { jest.clearAllMocks(); jest.spyOn(addressUtils, 'getSlicedAddress'); }); expect(addressUtils.getSlicedAddress).toHaveBeenCalledTimes(0);

package.json

wespickett · 2024-02-01T21:58:39Z

lgtm. We ended up switching to extend the next/jest config in build-onchain-apps, but if tests run successfully as is, then it should be fine with the current config https://github.com/coinbase/build-onchain-apps/blob/main/template/jest.config.js#L1

Zizzamia · 2024-02-01T23:34:35Z

src/components/OnchainName.tsx

+  }, [address, isLoading]);
+
+  if (isLoading) {
+    return null;


Not for later, this will cause a Layout Shift. Something to consider in follow up PR.

Zizzamia

BRILLIANT

alvaroraminelli added 8 commits January 30, 2024 16:45

feat: add new OnchainName component

54f9517

fix: ts config

e26f7fc

feat: ensname caching hooks

d73a7ab

chore: react testing library setup

a6cbf79

chore: install jest-dom

32b2f2f

chore: add unit tests for OnchainName component

5a0e400

chore: OnchainName documentation

0209c0b

remove jest setup from typescript

ad4cab3

Zizzamia reviewed Jan 31, 2024

View reviewed changes

alvaroraminelli added 3 commits January 31, 2024 12:37

fix: build failing

60fe0c0

fix unit tests setup

88cddc9

Merge branch 'main' into alvaroraminelli/feat-onchain-name

69e5b7d

alvaroraminelli marked this pull request as ready for review February 1, 2024 00:29

alvaroraminelli requested a review from Zizzamia February 1, 2024 00:30

Zizzamia reviewed Feb 1, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

Zizzamia reviewed Feb 1, 2024

View reviewed changes

README.md Show resolved Hide resolved

Zizzamia reviewed Feb 1, 2024

View reviewed changes

Zizzamia requested review from robpolak and cnasc February 1, 2024 02:39

cnasc reviewed Feb 1, 2024

View reviewed changes

alvaroraminelli added 5 commits February 1, 2024 10:53

addressing PR comments

6dac8b8

Merge branch 'main' into alvaroraminelli/feat-onchain-name

6fd3031

addressing PR review

4b3a61c

merge branch main

1de2c06

adding viem dependecy

ee2d749

alvaroraminelli commented Feb 1, 2024

View reviewed changes

package.json Show resolved Hide resolved

alvaroraminelli added 3 commits February 1, 2024 14:05

fix: not allow null addresses

edbf5b6

merge branch main

dcf4c9c

fixing peerDependecies

92e4aec

alvaroraminelli requested review from cnasc and Zizzamia February 1, 2024 22:18

Zizzamia reviewed Feb 1, 2024

View reviewed changes

Zizzamia approved these changes Feb 1, 2024

View reviewed changes

Zizzamia merged commit 8af8505 into main Feb 1, 2024
5 checks passed

Zizzamia deleted the alvaroraminelli/feat-onchain-name branch February 1, 2024 23:35

github-actions bot mentioned this pull request Feb 3, 2024

chore: version packages v0.4.2 🌊 #73

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: new OnchainName component #49

feat: new OnchainName component #49

alvaroraminelli commented Jan 31, 2024 •

edited

Loading

Zizzamia Jan 31, 2024

Zizzamia Feb 1, 2024

alvaroraminelli Feb 1, 2024

Zizzamia Feb 1, 2024

Zizzamia Feb 1, 2024

cnasc Feb 1, 2024

alvaroraminelli Feb 1, 2024

Zizzamia Feb 1, 2024

Zizzamia Feb 1, 2024

Zizzamia Feb 1, 2024

cnasc Feb 1, 2024 •

edited

Loading

cnasc Feb 1, 2024

alvaroraminelli Feb 1, 2024

Zizzamia Feb 1, 2024

alvaroraminelli Feb 1, 2024

Zizzamia Feb 1, 2024 •

edited

Loading

cnasc Feb 1, 2024

alvaroraminelli Feb 1, 2024

Zizzamia Feb 1, 2024

Zizzamia commented Feb 1, 2024

cnasc Feb 1, 2024

alvaroraminelli Feb 1, 2024

wespickett commented Feb 1, 2024

Zizzamia Feb 1, 2024

Zizzamia left a comment

	const normilizedAddress = useMemo(() => {
	const normalizedAddress = useMemo(() => {

feat: new OnchainName component #49

feat: new OnchainName component #49

Conversation

alvaroraminelli commented Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

If OnchainName accepts undefined address

If OnchainName rejects undefined address

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnasc Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zizzamia Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zizzamia commented Feb 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wespickett commented Feb 1, 2024

Choose a reason for hiding this comment

Zizzamia left a comment

Choose a reason for hiding this comment

alvaroraminelli commented Jan 31, 2024 •

edited

Loading

If `OnchainName` accepts undefined `address`

If `OnchainName` rejects undefined `address`

cnasc Feb 1, 2024 •

edited

Loading

Zizzamia Feb 1, 2024 •

edited

Loading