Skip to content

ENS Names used in Zones page and Account panel #1407

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

Merged
merged 5 commits into from
May 24, 2024
Merged

Conversation

Steedie
Copy link
Contributor

@Steedie Steedie commented May 13, 2024

What

Uses ENS name if it can be found instead of the wallet address:

Zones Page

image

Account Panel

image

Notes

  • I've tested to see that it's working on the account panel
  • I don't know 100% if it's working on the zone page until it's in main because I can't mint a zone on my MetaMask account locally (I think?)
    • However, I can get it to log my ENS on the zones page, so it is working.

@hypnoshock
Copy link
Contributor

I think you should be able to test it locally by adding one of the Anvil accounts to metamask and transferring some of the test ETH to your account. That way you'd be able to mint your own zone. Outside of that, another option could be to mint the zone using the dev account and then transfer the zone 721 to your account by adding the zone721 contract to your Metamask. I think the transfer of fake eth is the easier option though

Copy link
Contributor

@chrisfarms chrisfarms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we confirm how this performs when there are 100s+ of zones with unique addresses?

the page receives data updates multiple times a second, and if each of those updates causes an async external request for hundreds of addrs, it could get hairy.

ideally:

  • the lookup would only occur for addresses that are on the screen (or soon will be)
  • the lookups are cached so that the same address is not fetched multiple times whenever the component data changes
  • we do not send out more than say 5 concurrent requests at a time

}));
}
});
}, [zones]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this hook might get invalidated much more frequently than we would like for the kind of expensive lookup that is happening.

🤔

@Steedie
Copy link
Contributor Author

Steedie commented May 14, 2024

Could it be better to store it in something like zone.owner.ens when the zone is minted, then update it on their side whenever they connect or join their zone?

@chrisfarms
Copy link
Contributor

nah i think you are on the right track with the fetching on demand, we just need to be careful with creating a storm of requests

Steedie added 2 commits May 15, 2024 18:40
still TODO: only lookup addresses for zones that are visible on the screen
@Steedie
Copy link
Contributor Author

Steedie commented May 16, 2024

Since latest commit(s):

  • implemented address/ens caching (until page refreshed)
  • lookupENSName can only be used by up to 5 zone rows at a time
  • only calls lookupENSName on zone rows that are visible on the screen

Linting still todo

@Steedie
Copy link
Contributor Author

Steedie commented May 20, 2024

linted and ready

Copy link
Contributor

@hypnoshock hypnoshock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this solves the problem of scale that Farms pointed out so LGTM. A comment I have which I think is separate from this PR is about the zone list updating every 2 seconds. I'm not sure if Farms fixed this when he did the big CSS overhaul but in the old version (that I was responsible for 😬) it would rerender all the zone rows each time the zone data came in. I guess we'd need to memoize the zone data as it shouldn't be frequently updating

@Steedie
Copy link
Contributor Author

Steedie commented May 20, 2024

I think this solves the problem of scale that Farms pointed out so LGTM. A comment I have which I think is separate from this PR is about the zone list updating every 2 seconds. I'm not sure if Farms fixed this when he did the big CSS overhaul but in the old version (that I was responsible for 😬) it would rerender all the zone rows each time the zone data came in. I guess we'd need to memoize the zone data as it shouldn't be frequently updating

ty :)

Ah, this reminds me that I can remove the zones dependency. (latest commit: 8d842a9)

@ldunnplaymint ldunnplaymint merged commit 90bdf3a into main May 24, 2024
3 checks passed
@ldunnplaymint ldunnplaymint deleted the ens-names branch May 24, 2024 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants