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

Update ENS Resolver ABI #2700

Merged
merged 6 commits into from
Jul 7, 2023
Merged

Update ENS Resolver ABI #2700

merged 6 commits into from
Jul 7, 2023

Conversation

Snedashkovsky
Copy link
Contributor

@Snedashkovsky Snedashkovsky commented Oct 30, 2022

What was wrong?

Deprecated Resolver ABI

Related to Issue #1839, #1397

How was it fixed?

@Snedashkovsky

  • Update of Resolver ABI to be more generic. Research actual resolver contracts and create a more generic ABI here

@fselmo

  • Match ENS's Public Resolver 2 ABI as the default along with the Extended Resolver functions. I'd rather not have functions like renounceOwnership() available without the proper documentation and the proper UX, with aliased methods that are clear on what they do.

Todo:

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@fselmo
Copy link
Collaborator

fselmo commented Jul 6, 2023

Thanks @Snedashkovsky so much for the work here. This was needing some love. And sorry for the delay getting here.

I don't feel great about adding in the Ownable contract with renounceOwnership(), etc... without the proper documentation and a better UX for it. Seeing as the ENS Public Resolver 2 (current iteration of the public resolver) doesn't have this either, I think we leave the default ABI as the ENS: Public Resolver 2 ABI and extend it with the extended resolver methods.

I feel like this is a nice default and users can always customize and set a different ABI for their resolver if they have one to load. Any thoughts there?

I rebased as well since the tests were failing 👀

@fselmo
Copy link
Collaborator

fselmo commented Jul 6, 2023

cc: @pacrob @kclowes @reedsa

@fselmo fselmo requested review from kclowes, pacrob and reedsa July 6, 2023 23:26
@Snedashkovsky
Copy link
Contributor Author

@fselmo thank you for the PR update!
LGTM

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.

2 participants