Add ReverseRegistrarV2 #125
Conversation
✅ Heimdall Review Status
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
#123 now depends on this contract since we have wrapped the 2-step reverse set in a single call in reverse registrar |
| /// @notice Interface for the L2 Reverse Registrar. | ||
| interface IL2ReverseRegistrar { |
There was a problem hiding this comment.
if this is the one defined by ENS, would appreciate a callout and/or link to their repo
There was a problem hiding this comment.
added link to PR. will update with link to contract when it's merged.
| import {Sha3} from "src/lib/Sha3.sol"; | ||
|
|
||
| contract SetNameForAddrWithSignature is ReverseRegistrarV2Base { | ||
| function test_setsNameForAddrWithSignature() public { |
There was a problem hiding this comment.
Would prefer fuzzing on tests. In this case, seems like we could fuzz most args and generate signatures with a util to validate. Ideally we could also have an integration test against the deployed L2ReverseRegistrar but that doesn't exist yet right? For now maybe just having some basic fuzzing would be nice.
There was a problem hiding this comment.
Though I generally agree that fuzzing should be applied liberally, I think fuzzing around signature validation is out of scope for this contact. We rely on an external contract to do the signature validation so it doesn't make sense to me to add that validation to this repo's scope.
I agree an integration test would be ideal. We can technically do this against testnet today (with an older version) but I think a more valid integration test would use mainnet's deployment when it goes live. Thoughts on the specifics here?
There was a problem hiding this comment.
We can still fuzz the other function arguments though? Aligned that we don't need to test our dependency's signature validation, but it would also be nice to have a test that validates our assumptions of what happens if signature validation does fail on the L2ReverseRegistrar which we could mock.
As for integration tests, I took a look at the ENS PR and one thought could be changing our lib dependency to the branch so we can access their code. This would create future work for us though to eventually set branch back to main or some new feature branch if they merge and then change things.
Another option could be building the raw bytecode for their current sample implementation and etch that code manually. We'd need to update the bytecode manually as they push new updates but would decouple from other logistical weirdness.
Am fine for now adding a todo linear ticket so we don't forget to do something when it's more convenient.
There was a problem hiding this comment.
I like the idea of circling back once ENS deploys and writing some fork integration tests around their contract.
Added tests. Also found a bug where the helper |
| import {Sha3} from "src/lib/Sha3.sol"; | ||
|
|
||
| contract SetNameForAddrWithSignature is ReverseRegistrarV2Base { | ||
| function test_setsNameForAddrWithSignature() public { |
There was a problem hiding this comment.
We can still fuzz the other function arguments though? Aligned that we don't need to test our dependency's signature validation, but it would also be nice to have a test that validates our assumptions of what happens if signature validation does fail on the L2ReverseRegistrar which we could mock.
As for integration tests, I took a look at the ENS PR and one thought could be changing our lib dependency to the branch so we can access their code. This would create future work for us though to eventually set branch back to main or some new feature branch if they merge and then change things.
Another option could be building the raw bytecode for their current sample implementation and etch that code manually. We'd need to update the bytecode manually as they push new updates but would decouple from other logistical weirdness.
Am fine for now adding a todo linear ticket so we don't forget to do something when it's more convenient.
https://linear.app/coinbase/issue/BA-1952 added to track |
To fully support ENSIP-19, two data migrations are needed:
To address number 1, an owner-only hook has been added which allows the basenames admin to write ENSIP-11 data on behalf of existing basenames.
To address number 2 and to ensure data is not lost in the migration process, the reverse data will be written during the migration period to both the legacy resolver and to the new ENS contract.