-
Notifications
You must be signed in to change notification settings - Fork 497
Add ReverseNamer
#482
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
base: staging
Are you sure you want to change the base?
Add ReverseNamer
#482
Conversation
|
I like it! Should this PR also delete ReverseClaimer.sol, or maybe more safely just add a natspec comment indicating to use ReverseNamer.sol for wider support (rollups)? |
|
@adraffy Whats the difference between case 2 and case 3 Would it makes sense to just have 2 cases? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR looks good, thanks for this. We as Enscribe team also need to push more on awareness so developers include this in their contracts before deploying.
Added a few comments
|
|
||
| /// @notice https://docs.ens.domains/ensip/19/ | ||
| library ReverseNamer { | ||
| address constant MAINNET = address(0); // TODO: ENSv2 addr.reverse registrar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to add TESTNET or is it same as MAINNET address? since we have distinct MAINNET_ROLLUP and TESTNET_ROLLUP
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will mean all mainnets (mainnet/sepolia/...).
I speculatively designed ETHReverseResolver as the v2 replacement for addr.reverse that uses the same tech as the rollups (independent registry + wildcard resolver) but I think we're going with a proper claimable {addr}.addr.reverse that uses the v2 design, but it should be a fixed address deployment and the same on those chains. And If not, you're correct.
| return MAINNET_ROLLUP; | ||
| } else if (isTestnetRollup(chainId)) { | ||
| return TESTNET_ROLLUP; | ||
| } else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on line 10 - MAINNET = address(0) so is the else case
| } | ||
|
|
||
| /// @notice Mixin for delegated contract naming. | ||
| contract NameableBy is NotOwnable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NotOwnable in case of NamedOnce makes sense as no one can later name the contract but in case of NameableBy doesnt make much sense to it.
NameableBy can be seen in a contract with Ownable as an extra delegate who can name contract but still if contract has Owner he can revoke the delegation. I might be wrong but limiting NameableBy to only non ownable contracts doesn't make much sense to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I wasn't sure about this design. NameableBy could be removed. My thinking was you want something like Ownable but without implementing Ownable which at a glance implies the contract is mutable.
NameableBy + Ownable would give you 2 accounts that can change the name, but maybe that should be allowed.
I was thinking of it more like a trust curve:
NamedOnce— set onceNameableBy— set later, revoke it / delegate to someone to name itNameableBy— delegate to someone, change wheneverOwnable— change whenever, but shares with otherOwnableresponsibilities
NamedOncefor simple immutable name on constructorNameableByfor delegated namingReverseNamer.setName("<name>")for custom implementationUsage:
Alternative ideas:
NamedOnce→emit Named(string)→ generate proof of logaddress(<registrar>).call(abi.encodeWithSignature("setName(string)", "<name>"))