Include a reverse registrar when deploying a dev registry #5763
Conversation
haltman-at
requested review from
gnidan,
benjamincburns,
cds-amal and
eggplantzzz
|
OK after talking to @gnidan it seems there's probably no need for the "default resolver" to be the resolver of (Edit: Or does it? I guess it does, but then I have to rely less on our existing methods? Oy.) (Also, we should probably stop giving everything a separate resolver? But that can be a separate PR.) |
packages/deployer/ens.js
Outdated
| @@ -118,6 +185,35 @@ class ENS { | |||
| } | |||
| } | |||
|
|
|||
| //this method assumes that the current owner is `from`, | |||
| //and sets the new owner to be `owner`. | |||
| async alienateNameOwner({ name, from, owner }) { | |||
There was a problem hiding this comment.
would something like changeNameOwner or updateNameOwner be a more appropriate name for this method?
There was a problem hiding this comment.
Well, the point is that this is for giving it away, given that you currently own it. I'm not sure that it would work otherwise. So it could be called giveAwayNameOwner or something, but I wouldn't want to just use the word "change" or "update"; that doesn't seem specific enough.
There was a problem hiding this comment.
Isn't this method really just something that takes the current owner and a new owner and then changes ownership?
There was a problem hiding this comment.
No, it assumes you already own it. Otherwise you're not going to be able to give it away!
That said, if I redo this the way you suggested below, this method is likely just going to go away, replaced by more direct calls.
There was a problem hiding this comment.
OK, I ended up changing this to a more general update method since there's no real need to do the tree stuff. Yay simplicity!
| //here, however, we have to go *up* the tree. | ||
| //as such, we are going to process the labels in order, rather | ||
| //than in reverse order. | ||
|
|
There was a problem hiding this comment.
maybe you said in the description that it is not necessary, but for completeness I'm going to ask, "why do we need to swap owners all the way up the tree?" I mean it's not exactly arbitrary who owns all those names? Every time you use this method all of the names that are subsets of the name you are concerned with will change owners, no?
There was a problem hiding this comment.
Yeah, I'm really not sure we do! This is one of the ways that I'm worried that I overcomplicated this!
I mean I guess I just did it this way to keep things parallel and avoid having to write too much new code that doesn't parallel the old code? But probably this is overcomplicated and should be changed. I guess I'll go do that?
packages/deployer/ens.js
Outdated
| //(if there were a previous reverse registrar, this would happen automatically, | ||
| //but there wasn't, so it doesn't.) | ||
| //so, first let's claim the name for ourself again | ||
| await this.setNameOwner({ from, name: reverseResolutionSpecialName }); |
There was a problem hiding this comment.
why do we do this and then change it again with alienateNameOwner directly afterwards? can we just change it directly with one call? or maybe there is something I'm missing about these steps
There was a problem hiding this comment.
This is another way that I'm worried I overcomplicated things. :) I think if we didn't give away "reverse", like you suggest above, this wouldn't be necessary. Since we did give it away, that leads to this mess where we have to get ti back rather than directly change things...
I guess I'll go change this to get rid of all of these unnecessary transfers.
|
OK, I've updated this to make it simpler and reduce unnecessary transfers. I also decided to just use literal |
This PR makes it so that, when the deployer deploys a dev registry, it also deploys a reverse registrar and hooks it up to the dev registry for use.
This turns out to be quite a pain! Man, nothing with ENS can ever be simple, can it? Also, while I've hooked up the reverse registrar, I haven't actually really exposed it for use in the ENS interface; if you want to use it, you're going to have to break the abstraction and use
deployer.ens.ensjsyourself.Why didn't I add an interface for setting a reverse record? Basically, because any interface I added would have to be terrible! ENS only lets you set a reverse record for the address that is sending the transaction. With such a limitation, I was like, what interface can we even reasonably put on this?? Well, uh, let me know if you have a suggestion...
Given that, you might wonder why this is even useful. Well, um, I want it so I can test out my work on reverse ENS resolution in the debugger. And, uh, likely someone else will want it at some point? IDK.
Btw, while I was at it, I noticed that our
ENSRegistryandPublicResolverartifacts were out of date, so I updated those to the latest versions. That's in a separate commit, of course.Anyway, how does this PR work?
Well, as the final step in setting up a dev registry, we now call a new method I've added,
deployNewDevReverseRegistrar. Of course, calling this a "step" is a bit of a stretch... it's quite a series of steps, actually.You see, in order to deploy a reverse registrar, we need some pieces of information; we need the address of the registry, and we need the address of the default resolver. I'm, uh, actually not sure what a default resolver is; but since the way you become the reverse registrar is by claiming the address
"addr.reverse", I'm guessing the resolver of that address will do?So, OK. We need to deploy a resolver for that address, which means we need to own that address. But then to deploy the reverse registrar, we need to not own
"addr.reverse"; we need the registry to own it. So we claim"addr.reverse", deploy a resolver for it and set its resolver to that, but then give away"addr.reverse"to the registry, and only then do we deploy the reverse registrar.Except, now we need to hook up the reverse registrar -- if there were a previous reverse registrar this would happen automatically, but there isn't, so we need to do it. To do that we need to give ownership of
"addr.reverse"to the reverse registrar, so we claim that address back from the registry before giving it away again to the reverse registrar. (Why claim it back first before giving it away again? See below.) Whew!I think I probably made some of this more complicated than it needed to be. In particular, the key thing here is who owns the address
"addr.reverse". Note that it does not matter who owns"reverse", only"addr.reverse"!Why do I mention this? Because, you see, our method for claiming a name assumes you want to claim everything on the way to it as well -- it doesn't just claim
"addr.reverse", it claims"reverse"for you as well. Is doing that really necessary here? Not really, but I wanted to keep things consistent, and I assumed there was probably a good reason for this, so I stuck with it.Except, the thing about doing that, is that it complicates giving the address away. If you use such a complicated method for claiming addresses, then you also have to use a complicated method for alienating addresses, which I had to add.
Or at least... you have to do that if you make the assumption that I did, that when you give away an address, you also want to give away everything on the way to it. And I'm realizing now that that assumption maybe doesn't actually make sense! I can see why it makes sense for claiming addresses, but does it really make sense for alienating them? Maybe not! Oh well -- I only just realized that, I did it the complicated way. That's what review is for, I guess!
(This is also what necessitates claiming back
"addr.reverse"before giving it away again; this isn't necessary if you're just dealing with the address itself, but if you're dealing with everything above it too...)Anyway, yeah, after going through this whole dance, we do ultimately have a reverse registrar you can use. I mean, you can use it if you're wiling to ignore our abstractions and do things manually. But it's there, so that's something! Geez this was annoying. Well, you all can tell me what you think and if you think any of this should be simplified.
Finally, formalities -- regarding testing, I added two tests to
deployer/test/ens.jsthat I think adequately test this functionality, so you can just run those. I don't think this requires updating the docs, although, not sure, maybe it does? And I don't think this counts as a new feature for@truffle/deployer, but maybe someone disagrees.