-
Notifications
You must be signed in to change notification settings - Fork 15
feat: ENSAdmin - indexed chains panel UI improvements #740
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
feat: ENSAdmin - indexed chains panel UI improvements #740
Conversation
🦋 Changeset detectedLatest commit: 29a2e99 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 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 |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
…/feat-ensadmin-indexed-chains-panel-ui-improvements
… (if chain supported)
For whoever may reviewI am a little confused about the display of block's timestamp. In the I was unsure what the right approach here was, and continued with the "static" display for the relative timestamp. |
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.
@Y3drk Hey great job! 🚀 Reviewed and shared some suggestions with feedback 👍
|
|
||
| if ( | ||
| block.timestamp <= Math.floor(Date.now() / 1000) || | ||
| calculatedRelativeTime === "less than a minute ago" |
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.
Could you please investigate why both of these blocks are appearing like they have the same relative timestamp?
They have different absolute timestamps as can be confirmed by searching for these block numbers on Basescan: https://basescan.org/
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
I investigated on such Base blocks:
- 30742968 - index time: May-26-2025 04:01:23 PM +UTC
- 30742961 - index time: May-26-2025 04:01:09 PM +UTC
The time difference between them is 14s, so the fact that both of them had just now relative timestamp was in line with the code at the time.
Why? Because of how we set up the formatRelativeTime function and the formatDistanceToNow helper function inside it. In the version I used till now, it would unify all timestamps that mark time under a minute ago to less than a minute ago, which would then be caught in the component and simplified to just now.
But your conclusion that the timestamps differ and it should be visible in the panel is very much valid, hence I made some changes, and now formatRelativeTime will accept an optional argument that will be responsible for injecting an includeSeconds option into the helper function, which will in turn return a more fine-grained relative timestamp.
Summing up:
- The timestamps will now be more fine-grained and will include texts such as
less than X seconds agowhich should make the panel more detailed and less boring (multitude of descriptions rather thanjust noweverywhere) - The
just nowtext will now be solely reserved for the future edge case you described in the issue. - The optional argument allows backwards compatibility so there is no need to change any code from
recent-registrationscomponent that also uses theformatRelativeTimefunction
Below I'll paste how the panel looks with the aforementioned changes:

| if (blockExplorerAvailableChains.includes(networkName)) { | ||
| return ( | ||
| <a | ||
| href={`https://www.blockexplorer.com/${networkName.toLowerCase().replace(" ", "")}/block/${block.number}/#overview`} |
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.
We should use the "de-facto official" chain explorer for each blockchain.
I think @tk-o may have previously written some utility function for this somewhere? Can you please coordinate with him for some advice on this? Thanks!
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.
Spoke with @Y3drk about using the Datasource.chain getter to access selected chain configuration object defined by viem: Chain type.
Please note, the Chain value is not fully validated, i.e. it's type allows undefined block explorer.
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
Indeed, there was something similar. I got some good advice from Tomasz and applied it in the new solution that I'll describe in the comment about new chain-icon mapping strategy.
| network: NetworkStatusViewModel; | ||
| } | ||
|
|
||
| type chainName = |
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.
Suggest we take a different strategy for this:
- We should pass chainId numbers around, rather than chain name strings. The issue with chain name strings is it creates interop problems: Is it "OP Mainnet" "Optimism" "Optimism Mainnet", etc..? But with a chain id number all can easily agree with interop.
- We should have a function that takes in a chain Id number and then returns the appropriate chain name for use in our UI.
- We should have a function that takes in a chain id number and then returns the appropriate icon for use in our UI.
- Both of the functions described in 2 and 3 above should throw an error if they see a chain id number they don't have predefined answers for.
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 2nd point of your suggestion is already implemented via globalIndexingStatusViewModel function and getChainById helper function it uses, hence preventing incidents you described in 1st bullet from happening.
I've implemented the 3rd point of the suggestion with chainIcons map and getIconByChainId function.
To avoid boilerplate and limit prop drilling I've added two new fields to NetworkStatusViewModel interface, that is id:number which is chain's numerical identifier and blockExplorer an optional field that takes its structure directly from viem's Chain type.
Since these changes only add new fields (one of them even optional), I expect them to be backwards compatible.
I also updated unit tests for view-models.ts to accommodate for these changes, and the updated versions passed. I tested as thoroughly as I could to confirm that none of these changes break the app, and I am 99.99% sure of it.
These changes also simplified the display of the block's number as a link to chain's block explorer, addressing another comment on this PR.
Since these structures were designed by you, @tk-o, I'd appreciate it if you could confirm the described above changes as legit or share some suggestions on how to improve them.
| | "Ethereum" | ||
| | "Linea Mainnet" | ||
| | "Sepolia" | ||
| | "Anvil" |
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.
Please check with Tomasz and Matt what name is appropriate to give to a local ethereum. I'm not sure it is fair to always call it "Anvil" and maybe we need a more generic term.
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.
We can leverage viem's native configuration object for various chains. For example:
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.
Since https://github.com/wevm/viem/tree/main/src/chains/definitions contain definition for Anvil and lack some obvious generic replacement for it, I stuck with the current naming convention, all while applying new chain-icon approach that you proposed in the other comment.
@tk-o
If I missed some blatantly obvious replacement let me know :)
…uggested refinements from PR review
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.
@Y3drk Thanks for the updates 👍 Reviewed and shared suggestions
…trings, move getChainName to datasources package + beginning of refactors of block exploer & relative time helper functions
…helper functions + refactor of getBlockExplorerUrl function
…Ls for supported chains.
…/feat-ensadmin-indexed-chains-panel-ui-improvements
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.
@Y3drk Thank you. Approved. 👍 Shared a few small suggestions. Please take the lead to merge this when ready.
| * Chain id standards are organized by the Ethereum Community @ https://github.com/ethereum-lists/chains | ||
| */ | ||
| const chainIcons = new Map<number, React.ReactNode>([ | ||
| [mainnet.id, <EthereumIcon width={18} height={18} />], |
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 is a nice way to define the keys for one of these maps. Why aren't we doing the same in the other maps?
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.
Perhaps I misunderstood your previous suggestions regarding these maps. Cause I assumed we didn't want the direct imports from viem used in there... Seems I was mistaken. My bad.
Will rollback chainNames and chainBlockExplorers maps to use these. That should also satisfy your request for simplification
…/datasources, minor styling adjustments, fix of formatRelativeTime function + minor docstrings changes

Closes #506 , which includes: