Skip to content
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

added ENS resolver hooks, display results in the console (Ready for review) #105

Merged
merged 17 commits into from
Aug 19, 2022

Conversation

Seroxdesign
Copy link
Contributor

@Seroxdesign Seroxdesign commented Jul 4, 2022

Progress

  • Added two functions in the hooks folder. one resolves ENS to address and one resolves address to ENS

Remaining work

What's left:

  • Adding checks for search terms to ensure we call the right method
  • Adding a debounce to minimise spam to RPC
  • Showing results under search bar and on the account view page

@vercel
Copy link

vercel bot commented Jul 4, 2022

@Seroxdesign is attempting to deploy a commit to the Superfluid Finance Team on Vercel.

A member of the Team first needs to authorize it.

@Seroxdesign Seroxdesign changed the title added ENS resolver hooks, display results in the console added ENS resolver hooks, display results in the console (WORK IN PROGRESS) Jul 4, 2022
@Seroxdesign
Copy link
Contributor Author

Seroxdesign commented Jul 4, 2022

ready for review

@Seroxdesign Seroxdesign changed the title added ENS resolver hooks, display results in the console (WORK IN PROGRESS) added ENS resolver hooks, display results in the console (WIP) Jul 4, 2022
@kasparkallas
Copy link
Collaborator

kasparkallas commented Jul 5, 2022

Glad to see interest in contributing to the Console!

Some comments on how to solve this story:

  • We use RTK-Query to handle cache'ing and some request debounce'ing. Ethers' JsonRpcBatchProvider & FallbackProvider are used for further throttling & robustness.
  • We implemented a solution for the upcoming Dashboard and would like to re-use it in the Console. If the solution is not good then we'd like to know why. I put the snippets of the code in another ENS pull request: feat(console): add ens domains #88 (comment)
  • Effectively this bounty can be grabbed by just following these snippets.

@Seroxdesign
Copy link
Contributor Author

Got it! Thank you Kasper

@Seroxdesign
Copy link
Contributor Author

Still reading through all the snippets, and deciding how to fit them in. Expecting results by July 9th.

@Seroxdesign
Copy link
Contributor Author

@kasparkallas @vmichalik @sunnyjaycer

Ready for review

@Seroxdesign Seroxdesign changed the title added ENS resolver hooks, display results in the console (WIP) added ENS resolver hooks, display results in the console (Ready for review) Jul 7, 2022
@vercel
Copy link

vercel bot commented Jul 12, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
superfluid-console ✅ Ready (Inspect) Visit Preview Aug 19, 2022 at 11:50AM (UTC)

Copy link
Collaborator

@kasparkallas kasparkallas left a comment

Choose a reason for hiding this comment

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

Did a quick tour and seems to work overall! 👍

Couple of comments:

  • doesn't show spinner when still fetching ENS
  • when on an account page and then going to another account's page then previous avatar is shownfor a short period of time (use RTK-query's .currentData over .data or set a key for the page [use address as key for example])
  • when both ENS and address book name are specified then the behaviour is weird with both showing and the address not showing
  • UI is a bit messy and doesn't follow https://mui.com/material-ui/

src/components/AddressBook.tsx Outdated Show resolved Hide resolved
src/hooks/useAddressENS.tsx Outdated Show resolved Hide resolved
src/pages/[_network]/accounts/[_id].tsx Outdated Show resolved Hide resolved
src/redux/slices/addressBook.slice.ts Outdated Show resolved Hide resolved
src/redux/store.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@Seroxdesign
Copy link
Contributor Author

I believe most problems have been resolved, although I might need to figure out how to display addressBook name and ENS name

@Seroxdesign
Copy link
Contributor Author

Did a quick tour and seems to work overall! 👍

Couple of comments:

* doesn't show spinner when still fetching ENS

* when on an account page and then going to another account's page then previous avatar is shownfor a short period of time (use RTK-query's `.currentData` over `.data` or set a _key_ for the page [use address as key for example])

* when both ENS and address book name are specified then the behavior is weird with both showing and the address not showing

* UI is a bit messy and doesn't follow https://mui.com/material-ui/
  • Switched to using currentData

  • Added conditionals to avoid rendering both address book name and ens name at the same time

  • Slightly improved UI on account page

  • Spinner isn't loading immediately because it takes a second to resolve ENS name to address for search

@kasparkallas Let me know if anything else needs work

@Seroxdesign
Copy link
Contributor Author

So seems like there's an issue I am not handling right, I don't know what the best way to check name before I resolve to address

@Seroxdesign Seroxdesign changed the title added ENS resolver hooks, display results in the console (Ready for review) added ENS resolver hooks, display results in the console (WIP) Jul 13, 2022
@Seroxdesign Seroxdesign changed the title added ENS resolver hooks, display results in the console (WIP) added ENS resolver hooks, display results in the console (Ready for review) Jul 14, 2022
@vmichalik vmichalik self-requested a review July 18, 2022 16:46
Copy link
Contributor

@vmichalik vmichalik left a comment

Choose a reason for hiding this comment

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

looks good to me!

@elvijsTDL
Copy link
Contributor

elvijsTDL commented Jul 18, 2022

The non-ens address in at the top of the page is alligned a little bit off now and it is smaller, here it is now
Screenshot 2022-07-18 at 21 20 08

And here is the before
Screenshot 2022-07-18 at 21 21 37

@vmichalik Should we also include showing the ENS names in the tables and using them in filters in this ticket?

Copy link
Contributor

@elvijsTDL elvijsTDL left a comment

Choose a reason for hiding this comment

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

Was on a slower network and the loading spinner doesn't show up when the ENS lookup is happening, actually thought the ENS lookup didn't work on Brave and Firefox because of that, could we please have one while the lookup is still happening , here is a video:

Screen.Recording.2022-07-18.at.21.40.42.mov

Also address book entries now show the input string too instead of just the result:
Screenshot 2022-07-18 at 22 00 44

@Seroxdesign
Copy link
Contributor Author

I can push a fix for these tomorrow morning, thanks for taking the time to review

@Seroxdesign
Copy link
Contributor Author

The non-ens address in at the top of the page is alligned a little bit off now and it is smaller, here it is now Screenshot 2022-07-18 at 21 20 08

And here is the before Screenshot 2022-07-18 at 21 21 37

@vmichalik Should we also include showing the ENS names in the tables and using them in filters in this ticket?

I changed the size of the text to comply with the ENS frontend standard,

image

An example of how this is relevant, I wanted the ENS to be bigger than the address, although this can be changed back to the original style

@Seroxdesign
Copy link
Contributor Author

Was on a slower network and the loading spinner doesn't show up when the ENS lookup is happening, actually thought the ENS lookup didn't work on Brave and Firefox because of that, could we please have one while the lookup is still happening , here is a video:
Screen.Recording.2022-07-18.at.21.40.42.mov

Also address book entries now show the input string too instead of just the result: Screenshot 2022-07-18 at 22 00 44

The text is a simple fix, I will remove showing the text on top of the address and instead only showed related addresses on ENS search.

As for the spinner, I tried fixing this yesterday but I am still trying to figure out the best way to do this without changing how the spinner renders. I will try again today and if I can't find a solution I will request support

@Seroxdesign
Copy link
Contributor Author

Seroxdesign commented Jul 20, 2022

Before:

image
image

After:

image
image

What's left:

  • [] Figuring out how to show spinner correctly.

@Seroxdesign
Copy link
Contributor Author

Seroxdesign commented Jul 23, 2022

@elvijsTDL @kasparkallas I am having trouble with the spinner task, I would appreciate any pointer or advice,

@Seroxdesign
Copy link
Contributor Author

Was on a slower network and the loading spinner doesn't show up when the ENS lookup is happening, actually thought the ENS lookup didn't work on Brave and Firefox because of that, could we please have one while the lookup is still happening , here is a video:
Screen.Recording.2022-07-18.at.21.40.42.mov

Also address book entries now show the input string too instead of just the result: Screenshot 2022-07-18 at 22 00 44

image

Fixed using isFetching on ensQuery

@vmichalik
Copy link
Contributor

@Seroxdesign, we really appreciate your efforts- you took to the task quickly, were responsive to feedback and suggestions and kept making tweaks to improve, so I'm more than happy to see this bounty paid out.

With Dashboard V2 still under such active development we likely won't have time to do a Console release containing this feature for a few weeks. However, we hope you enjoy the bounty payout, learnt more about ENS and the SDK-Redux and stick around for future bounties too🥇

Will DM you on Discord for next steps

@elvijsTDL
Copy link
Contributor

LGTM 👍

* rename to clarify intent
* be more explicit with types
* remove unneccessary try-catches
Copy link
Collaborator

@kasparkallas kasparkallas left a comment

Choose a reason for hiding this comment

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

When there's an address book entry then ENS name is not displayed anywhere. Not sure if I like it... The ENS name should still be visible somewhere but let's handle that in the next iteration -- would like to get this merged. :)

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.

None yet

4 participants