Skip to content

ENS resolver upgrade implementation#151

Merged
mds1 merged 11 commits intomasterfrom
ens-resolver-upgrade-implementation
Apr 7, 2021
Merged

ENS resolver upgrade implementation#151
mds1 merged 11 commits intomasterfrom
ens-resolver-upgrade-implementation

Conversation

@mds1
Copy link
Collaborator

@mds1 mds1 commented Apr 3, 2021

Closes #113

Everything should be working, but admittedly there's a bit of complexity and lots of checks to this flow, so there may still be some bugs for edge cases—please test thoroughly!

@mds1 mds1 requested a review from apbendi April 3, 2021 17:35
@apbendi
Copy link
Member

apbendi commented Apr 6, 2021

Getting the following error trying to go through the setup flow with a new ENS name:

Screen Shot 2021-04-06 at 6 15 37 PM

Screen Shot 2021-04-06 at 6 15 53 PM

EDIT: resolved with yarn at the root dir

Copy link
Member

@apbendi apbendi left a comment

Choose a reason for hiding this comment

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

Looks great! Everything seems to work fine with no hiccups. Awesome work. I tried to put it t through all the various scenarios and it seemed to respond correctly. A couple of minor UX things:

In the scenario where the user is not using the Public Resolver, it might be nice to include a link to the record in question to the ENS app, namely: https://app.ens.domains/name/$DOMAIN.eth

Screen Shot 2021-04-06 at 6 24 08 PM


"Don't worry, this will not break anything!" Haha this sounds a git ominous. Maybe say something like, "the rest of your ENS configuration will be carried over, and you'll be able to continue using ENS like normal" or something along those lines? I'd also consider bolding "You'll now be asked to sign 3 transactions"

Screen Shot 2021-04-07 at 6 11 39 AM


Gas used for three steps for me:

setAuthorisation: 47,709
setStealthKeys: 84,265
setResolver: 31,203

That's 163,177 in total, or about $40 at current prices. Not great, but not as awful as I'd feared.

- Carousel is no longer swipeable
- Link user to their ENS name on the ENS app to configure it
- Change phrasing / add bold text on the 3 tx setup step
@mds1
Copy link
Collaborator Author

mds1 commented Apr 7, 2021

Implemented the changes above, so merging now

@mds1 mds1 merged commit 0cf5e04 into master Apr 7, 2021
@apbendi apbendi deleted the ens-resolver-upgrade-implementation branch July 11, 2022 13:01
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.

ENS Resolver Upgrade Implementation

2 participants