Consistify location of user ENS registry address setting

[haltman-at]

PR description

This PR addresses #5997. I'm not entirely happy with this one, but, well, I figured I ought to put it up as it is and we can iron out the problems in review.

Basically, I altered both Truffle Contract and Truffle Deployer to check 4 places for the ens registry address:

1. config.network_config.registryAddress
2. config.network_config.registry.address
3. config.ens.registryAddress
4. config.ens.registry.address

Note that specific network configs are checked before the overall ENS config, so that a specific network config can override the overall ENS config.

Now I say I altered Truffle Contract, but actually I didn't -- the easiest place to make these changes was actually the provisioner, rather than Contract itself. Not sure if that's appropriate, but I didn't want to deal with the complication of doing it a different way.

I say I'm not entirely happy with this one, and there are two reasons for that:

1. Right now, since values are just combined with ??, if you set config.ens.registryAddress, it's not possible to, say, set an explicit null in a network config to say that you shouldn't use the global registry for that specific network. I can imagine people might want this in deployer. So likely only undefined should defer to the next one, rather than null.
2. This PR is repeating the same code twice -- and if we add similar code to Reverse ENS resolution support in debugger and decoder #5895 as @gnidan has suggested, that'll make 3 copies. Moreover, if we account for point (1) just above, the code will get more complicated in all these locations. That suggests it should be factored. The question then is, where should such the factored version live? One possibility would be to add a getter for this in config like we have for various other properties of the config; it could be called config.ensRegistryAddress or something (config.networkRegistryAddress? IDK). The naming's a little ugly, but it seems probably better than the alternative?

Anyway, let me know what you think and what changes should be made to this.

Testing instructions

I added tests to deployer to test the code changes there. I didn't really directly test the provisioner changes, hm, maybe I should go back and do that... (adding tests there seemed pretty messy)

[haltman-at]

Hm, this seems to be failing CI for a reason I don't understand. Will have to take a look.

[cds-amal]

As you mentioned, this is not ideal. I think it would be better to have Config resolver fn ensRegistryAddress(networkName)

[haltman-at]

A config resolver function? What does that mean? (Just to be clear what I was talking about above having just a getter that returns the current registry address, it wouldn't take any arguments.)

[cds-amal]

ah shoot, CURRENT! I forgot it gets set, and the getter won't need a network name.

[gnidan]

2. The question then is, where should such the factored version live? One possibility would be to add a getter for this in config like we have for various other properties of the config; it could be called config.ensRegistryAddress or something (config.networkRegistryAddress? IDK). The naming's a little ugly, but it seems probably better than the alternative?

I think we should do .registry.address, because I don't want to preclude other registry settings in the future (besides address), and it seems like a registry namespace would be useful for that.

It's a bit tricky though... I think the right place for this ENS config is per-network (i.e., config.networks[config.network].ens), but does that pose other problems? Can we get away with only configuring ENS at the individual network level?

[haltman-at]

These are interesting thoughts on where the user settings in the config should live, but this doesn't answer the question of how to name the getter so as to avoid a naming conflict!

[gnidan]

After consideration and discussion, let's just make whatever getter that doesn't conflict with user settings. Maybe ensRegistry, or heck, I'm not even altogether that opposed to ensRegistryAddress

[haltman-at]

OK, I've updated this to use a config getter, even though it's ugly. I also did some manual testing of the contract parts.

Really this is still not the cleanest even with the factoring, because I'm still just plugging the getter in to the particular places early on, converting it to a value, rather than letting config pass through and usign the getter as late as possible. But whatever -- that can be a thing for the future, likely.

[cds-amal]

Looks good @haltman-at, though I have a few comments. I think the new settings behavior should be documented on the website, and, imho, linked to by the new exception error the code can produce.

[cds-amal]

Should the resolution order be called out?

[haltman-at]

I don't think the particular order is important as long as network-specific is placed above generic. You shouldn't be setting the same thing twice anyway, so to my mind there's no need to guarantee an order between them; if you set both and get an unpredictable result, that's your own problem for setting the same thing twice.

(To be clear I consider network specific and generic settings to be different -- that's not setting the same thing twice, and there guaranteeing the order matters.)

Changed my mind because of untested complexity

[cds-amal]

I think there should be some unit tests to guard ens precedence resolution.

[cds-amal]

This precedence resolution locks the API behavior and should be guarded by unit tests to maintain behavior to prevent introducing a subtle bug in future maintenances.

[haltman-at]

Again, as long as specific is before generic, I don't think there's a problem (see comments earlier). In any case I did add a test that tests the specific-before-generic behavior; I don't see a need to add a test for the remainder of the order and lock it in.

[cds-amal]

That makes sense, though It would be helpful to inform the user they're in undefined territory if they set both network specific and generic ens settings.

[haltman-at]

Oh, hm, yeah, we could throw an error in that case! I guess I'll go do that.

[haltman-at]

OK, I reworked the logic here to throw an error in that case. I did use copy/paste but I think it's fine, it's all right next to each other and it's just two.

[cds-amal]

Looks good! Thanks, @haltman-at