migrate setResolver
#248
migrate setResolver
#248
Conversation
commit: |
| name: string; | ||
| /** Contract to set resolver on - can be 'registry', 'nameWrapper', or an address of a namechain registry */ | ||
| contract: "registry" | "nameWrapper" | Address; | ||
| /** Resolver address to set */ |
There was a problem hiding this comment.
I'd recommend turning contract into optional "registry" | "name wrapper" and adding a new parameter called registryAddress to make this more descriptive and precise. You can use discriminated unions to keep it type safe.
There was a problem hiding this comment.
i think we should think about it more. i'll leave it as is for now
|
| } | ||
|
|
||
| // Handle namechain contracts | ||
| if (registryAddress && !isAddress(registryAddress)) { |
There was a problem hiding this comment.
Don't need to check if registryAddress exists. isAddres will return false if registryAddress does not exist. Also this condition will fail if registryAddress is empty.
TLDR. Just check if registryAddres is address.
| const baseParams = { | ||
| address, | ||
| functionName, | ||
| address: registryAddress!, |
There was a problem hiding this comment.
We want to avoid forcing the value or registryAddress. Should add a guard function before this is isAddress does not satisfy it.
e.g. function checkAddress(x: unknown): x is Address
migrate
setResolverto support namechain contracts while maintaining backwards compatibility.the function now accepts registry addresses in addition to 'registry' and 'namewrapper' string literals.
for namechain contracts, it uses
labelhashinstead ofnamehashand callssetResolver(tokenid, resolver)with the appropriate abi.should we extract the namechain abi snippet to a separate file like ../../contracts/namechain.js similar to how
nameWrapperhas? currently it's defined inline because its very simple.