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

add ENS support for address reverse resolution and avatar #470

Closed
wants to merge 3 commits into from

Conversation

frimoldi
Copy link

@frimoldi frimoldi commented Dec 7, 2021

ENS reverse resolution

By adding ENS reverse resolution, the AccountButton component now is able to display the ENS name and avatar image, instead of the Ethereum address.

image

Changes

  • added @ensdomains/ensjs dependency to handle ENS reverse resolution (eth address => ens name).
  • created Avatar shared component. this component will display the ENS avatar image if there's any, or Jazzicon otherwise.
  • updated RariContext to provide the resolved ENS name.

@transmissions11
Copy link
Collaborator

hey thanks for your contribution, but we're not comfortable merging this with such a huge package-lock diff, as you could be installing a malicious npm package somewhere in there...

if you'd like to revise this PR to have a more minimal package-lock diff that we can certify is safe, we'd be happy to merge this :)

@Rari-Capital Rari-Capital deleted a comment from vercel bot Dec 7, 2021
@Rari-Capital Rari-Capital deleted a comment from vercel bot Dec 7, 2021
@transmissions11 transmissions11 requested review from transmissions11 and jarbacoa and removed request for transmissions11 December 7, 2021 19:51
@frimoldi
Copy link
Author

frimoldi commented Dec 7, 2021

hey @transmissions11 , yeah that makes total sense. All I did was to include the @ensdomains/ensjs dependency, but I'll investigate why there's such a big diff on package-lock.

@transmissions11
Copy link
Collaborator

transmissions11 commented Dec 7, 2021

dw fren i trust you, but with all the frontend exploits going around recently we've gotta vet this stuff nonetheless 😅

as for why the diff is so large, i'd guess you're using an older version of NPM which uses a different package-lock format. try installing the version listed in the requirements README section.

@vercel
Copy link

vercel bot commented Dec 7, 2021

@frimoldi is attempting to deploy a commit to the Rari Capital Team on Vercel.

A member of the Team first needs to authorize it.

@frimoldi
Copy link
Author

frimoldi commented Dec 7, 2021

@transmissions11 I've regenerated package-lock with the right npm version, still huge though.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1550235526

  • 4 of 25 (16.0%) changed or added relevant lines in 5 files are covered.
  • 95 unchanged lines in 16 files lost coverage.
  • Overall coverage decreased (-1.1%) to 10.017%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/utils/shortAddress.ts 0 1 0.0%
src/utils/ens.ts 1 3 33.33%
src/components/shared/Avatar.tsx 1 5 20.0%
src/context/RariContext.tsx 1 15 6.67%
Files with Coverage Reduction New Missed Lines %
src/components/shared/GlowingButton.tsx 1 25.0%
src/rari-sdk/subpools/keeperdao.js 1 50.0%
src/hooks/useNoSlippageCurrencies.ts 2 50.0%
src/rari-sdk/subpools/fuse.js 2 84.62%
src/hooks/usePoolBalance.ts 3 52.0%
src/rari-sdk/cache.js 3 63.16%
src/rari-sdk/subpools/aave.js 3 11.76%
src/utils/bigUtils.ts 3 44.44%
src/rari-sdk/subpools/dydx.js 4 50.0%
src/rari-sdk/subpools/mstable.js 4 31.71%
Totals Coverage Status
Change from base Build 1537144180: -1.1%
Covered Lines: 973
Relevant Lines: 6947

💛 - Coveralls

@frimoldi frimoldi closed this Jun 1, 2022
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.

3 participants