feat: initialize ENSLabelHealer contract#2054
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
📝 WalkthroughWalkthroughAdds ENSLabelHealer: a UUPS upgradeable contract emitting LabelHealed events for SUBMITTER_ROLE accounts, with deploy/grant/revoke scripts, unit/fuzz/invariant tests, Foundry project config and remappings, git submodules, a discovery-based CI workflow, READMEs, and test gas snapshots. ChangesENSLabelHealer Contract System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR initializes the
Confidence Score: 5/5Safe to merge; all UUPS security findings were raised in prior review rounds and the contract logic is straightforward. The new contract is a simple permissioned event emitter with no token transfers, no storage of funds, and no complex state transitions. Role management, pause mechanics, and upgrade authorization all follow standard OpenZeppelin patterns and are well-tested. Prior review threads already captured the UUPS implementation guard and zero-address initializer concerns. The remaining new observations are minor: a stale gas snapshot and two API surface clarifications. contracts/ens-label-healer/src/ENSLabelHealer.sol — review the outstanding items from prior threads (implementation constructor guard, zero-address check on Important Files Changed
Sequence DiagramsequenceDiagram
participant Deployer
participant Deploy.s.sol
participant ENSLabelHealer_Impl
participant ERC1967Proxy
participant Owner
participant Submitter
participant Indexer
Deployer->>Deploy.s.sol: run() with OWNER_ADDRESS
Deploy.s.sol->>ENSLabelHealer_Impl: new ENSLabelHealer()
Deploy.s.sol->>ERC1967Proxy: new ERC1967Proxy(impl, initialize(owner))
ERC1967Proxy->>ENSLabelHealer_Impl: delegatecall initialize(owner)
ENSLabelHealer_Impl-->>ERC1967Proxy: owner set, UUPS ready
Owner->>ERC1967Proxy: grantSubmitter(submitter)
ERC1967Proxy->>ENSLabelHealer_Impl: delegatecall _grantRole(SUBMITTER_ROLE, submitter)
Submitter->>ERC1967Proxy: submit("vitalik") / submitBatch([...])
ERC1967Proxy->>ENSLabelHealer_Impl: delegatecall submit() [onlyRole + whenNotPaused]
ENSLabelHealer_Impl-->>Indexer: emit LabelHealed("vitalik")
Owner->>ERC1967Proxy: pause()
Submitter->>ERC1967Proxy: submit("foo") [reverts: EnforcedPause]
Reviews (7): Last reviewed commit: "add proxy address to .env.example" | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/contracts_ci.yml:
- Around line 11-15: The path filters currently only include "contracts/**" so
the workflow won't run when changes touch workflow or submodule metadata; update
the push and pull_request paths to also include the workflow and submodule files
(add entries for ".github/workflows/contracts_ci.yml" and ".gitmodules" — or
more generally ".github/workflows/**" and ".gitmodules") so edits to the
workflow or .gitmodules will trigger the contract CI job.
In `@contracts/ens-name-healer/.env.example`:
- Around line 1-15: Update the .env.example so that all environment entries are
exported and include ETHERSCAN_API_KEY: prepend export to ADMIN_ADDRESS,
SUBMITTER_ADDRESS, DEPLOYER_PRIVATE_KEY, SEPOLIA_RPC_URL, MAINNET_RPC_URL (and
any other entries) so they are available to subprocesses used by
forge/Deploy.s.sol (e.g. vm.envAddress("ADMIN_ADDRESS") and
vm.envOr("SUBMITTER_ADDRESS", ...)), and add an exported ETHERSCAN_API_KEY line
for the mainnet deployment flow.
In `@contracts/ens-name-healer/README.md`:
- Line 140: The README text incorrectly says `--verify` uploads to Etherscan
while the example uses `--verifier sourcify`; update the sentence to reference
Sourcify (or make it generic) so it matches the command—edit the description
mentioning `--verify` / `--verifier sourcify` to state upload goes to Sourcify
or rephrase to "uploads the source to the specified verifier (e.g., Sourcify or
Etherscan)" to cover both cases.
- Around line 113-116: The example uses keccak over an ABI-encoded string which
does not produce an EIP-137 ENS namehash, so the healed mapping lookup will
query the wrong storage slot; replace the keccak-based argument with an ENS
namehash (EIP-137) when calling healed(bytes32). Concretely, update the cast
invocation that currently uses `cast keccak $(cast abi-encode "f(string)"
"vitalik.eth")` to instead compute the namehash for "vitalik.eth" (e.g., via
`cast namehash "vitalik.eth"` or any EIP-137 namehash helper) and pass that
bytes32 into the `healed(bytes32)(bool)` call so the lookup targets the correct
mapping key.
In `@contracts/ens-name-healer/remappings.txt`:
- Line 2: The remapping entry
"@openzeppelin/contracts/=lib/openzeppelin-contracts-upgradeable/lib/openzeppelin-contracts/contracts/"
is pointing to a non-existent nested directory; update the remapping to the
actual location of OpenZeppelin contracts in the repo (for example change it to
the existing path without the extra "lib/openzeppelin-contracts" segment such as
"lib/openzeppelin-contracts-upgradeable/contracts/" or to
"lib/openzeppelin-contracts/contracts/" depending on which package is present),
ensure the string in the remappings.txt matches that existing directory, and
then re-run the build to verify imports like `@openzeppelin/contracts/`* resolve.
In `@contracts/ens-name-healer/script/Deploy.s.sol`:
- Around line 30-35: The deploy script currently calls
ENSNameHealer.initialize(admin) then attempts
proxy.grantRole(proxy.SUBMITTER_ROLE(), submitter) from the deployer EOA which
will revert when admin != deployer; fix by either (A) adding an initial
submitter parameter to ENSNameHealer.initialize and using bytes memory initData
= abi.encodeCall(ENSNameHealer.initialize, (admin, submitter)) so the submitter
is granted inside initialize, or (B) remove the
proxy.grantRole(proxy.SUBMITTER_ROLE(), submitter) branch entirely and let the
admin (DEFAULT_ADMIN_ROLE) grant SUBMITTER_ROLE afterward; update references to
ENSNameHealer.initialize, proxy.grantRole, and SUBMITTER_ROLE accordingly.
- Line 24: After reading admin via vm.envAddress("ADMIN_ADDRESS") in
Deploy.s.sol, validate that the returned admin is not address(0) and abort if it
is; add a guard (e.g., require/admin check that reverts with a clear message
like "ADMIN_ADDRESS must not be zero") immediately after the line that sets
address admin = vm.envAddress("ADMIN_ADDRESS") so the script never continues
with a zero DEFAULT_ADMIN_ROLE holder and breaks upgrades/role management.
In `@contracts/ens-name-healer/src/ENSNameHealer.sol`:
- Around line 64-69: The initializer function initialize(address admin) allows
address(0) which can permanently lock DEFAULT_ADMIN_ROLE-controlled features;
update initialize (the initialize function) to require admin != address(0) and
revert with a clear message before calling _grantRole(DEFAULT_ADMIN_ROLE,
admin), so the contract never grants admin to the zero address and avoids
irreversible lockout.
- Line 19: Add a constructor to the ENSNameHealer contract that immediately
calls _disableInitializers() to lock the implementation contract; specifically
add a constructor() in the ENSNameHealer contract body invoking
Initializable._disableInitializers() (or simply _disableInitializers()) so the
implementation cannot be initialized, keeping the rest of UUPS setup
(initialize, _authorizeUpgrade, etc.) unchanged.
In `@contracts/ens-name-healer/test/ENSNameHealer.invariant.t.sol`:
- Around line 165-169: The array literal for knownNodes in
ENSNameHealer.invariant.t.sol is misformatted and failing forge fmt; reformat
the bytes32[3] memory knownNodes block (the three keccak256 entries) to match
forge formatting (or run `forge fmt`) so CI passes, ensuring the lines for the
"eth", "vitalik.eth", and "com" keccak256 entries are properly indented and
wrapped per rustfmt-style layout.
- Around line 162-175: The invariant invariant_healed_isMonotone currently
checks only three hard-coded namehashes; change it to iterate the bounded set of
nodes the handler actually touches by adding tracking in ENSNameHealerHandler:
maintain a fixed-size array (or dynamic array with cap) plus a mapping to avoid
duplicates (e.g., handler.seenNodes and handler.hasSeen(bytes32)) and expose a
view accessor (e.g., seenNodes(), seenCount()). Update the handler's action
methods to record each namehash they operate on into seenNodes, and then modify
invariant_healed_isMonotone to loop over handler.seenNodes()/handler.seenCount()
calling handler.shadowHealedFor(node) and asserting healer.healed(node) to
ensure monotonicity for the actual exercised nodes.
In `@contracts/ens-name-healer/test/ENSNameHealer.t.sol`:
- Around line 42-80: These tests use generic vm.expectRevert() which hides why
the call failed; update each negative-path test
(test_submit_revertsForNonSubmitter, test_forceResubmit_revertsForNonSubmitter,
test_pause_revertsForNonAdmin, test_unpause_revertsForNonAdmin,
test_setHealedStorageDisabled_revertsForNonAdmin,
test_upgradeToAndCall_revertsForNonAdmin) to assert the exact revert
reason/selector (e.g., the AccessControl "missing role" revert selector or the
Pausable/Ownable revert string) by calling vm.expectRevert(...) with the
specific encoded selector/message that the healer contract emits for
unauthorized or paused states, so failures only pass when the expected
authorization/pausable error is thrown.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 77f028c6-42e5-477f-ae21-6ce1eb860874
📒 Files selected for processing (16)
.github/workflows/contracts_ci.yml.gitmodulescontracts/README.mdcontracts/ens-name-healer/.env.examplecontracts/ens-name-healer/.gas-snapshotcontracts/ens-name-healer/.gitignorecontracts/ens-name-healer/README.mdcontracts/ens-name-healer/foundry.tomlcontracts/ens-name-healer/lib/forge-stdcontracts/ens-name-healer/lib/openzeppelin-contracts-upgradeablecontracts/ens-name-healer/remappings.txtcontracts/ens-name-healer/script/Deploy.s.solcontracts/ens-name-healer/src/ENSNameHealer.solcontracts/ens-name-healer/test/ENSNameHealer.fuzz.t.solcontracts/ens-name-healer/test/ENSNameHealer.invariant.t.solcontracts/ens-name-healer/test/ENSNameHealer.t.sol
lightwalker-eth
left a comment
There was a problem hiding this comment.
@sevenzing Appreciate your updates here. We should take a more simplified strategy for this contract. Please see comments for details 👍
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
contracts/ens-label-healer/.env.example (1)
1-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick winVariables without
exportwon't be inherited byforgesubprocess.When users run
source .env && forge script ...(the typical workflow), shell variables withoutexportremain shell-local and are not available to child processes. Foundry'svm.envAddress("ADMIN_ADDRESS")andvm.envString("DEPLOYER_PRIVATE_KEY")read from the process environment, so they will silently receive empty/default values unless the variables are exported.✏️ Proposed fix
-ADMIN_ADDRESS= +export ADMIN_ADDRESS= -SUBMITTER_ADDRESS= +export SUBMITTER_ADDRESS= -DEPLOYER_PRIVATE_KEY= +export DEPLOYER_PRIVATE_KEY= -SEPOLIA_RPC_URL= -MAINNET_RPC_URL= +export MAINNET_RPC_URL= +export SEPOLIA_RPC_URL= -ETHERSCAN_API_KEY= +export ETHERSCAN_API_KEY=🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/ens-label-healer/.env.example` around lines 1 - 17, The .env example uses plain assignments so child processes (and Foundry's vm.envAddress/vm.envString lookups) won't see them; update the variables (e.g., ADMIN_ADDRESS, SUBMITTER_ADDRESS, DEPLOYER_PRIVATE_KEY, SEPOLIA_RPC_URL, MAINNET_RPC_URL, ETHERSCAN_API_KEY) to be exported (add the export keyword) so that forge subprocesses inherit them when running source .env && forge script ...; ensure the example shows exported variables and mention any required vs optional ones in the comments near the corresponding names.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.gitmodules:
- Around line 1-6: The .gitmodules entries for the submodules
"contracts/ens-label-healer/lib/openzeppelin-contracts-upgradeable" and
"contracts/ens-label-healer/lib/forge-std" are unpinned; update them to
reference a specific release (add a branch/ tag or commit SHA and/or an inline
comment with the tag) so the repo uses a reproducible version instead of master,
and ensure your remapping reference
(openidzeppelin-contracts-upgradeable/lib/openzeppelin-contracts) matches that
pinned release; additionally update the CI (actions/checkout) or pipeline steps
used for ENSLabelHealer.sol to initialize nested submodules by enabling
submodules: recursive on actions/checkout or running git submodule update --init
--recursive so the nested path required by remappings.txt exists during builds.
In `@contracts/ens-label-healer/README.md`:
- Around line 3-5: Replace the hyphenated "on-chain" occurrences with the
repository-preferred single word "onchain": update the README phrase
"Permissioned on-chain label emitter for unresolved ENS labels." to
"Permissioned onchain label emitter for unresolved ENS labels." and change
"publish labels on-chain via `LabelHealed(string label)`" to "publish labels
onchain via `LabelHealed(string label)`".
In `@contracts/ens-label-healer/remappings.txt`:
- Around line 1-3: The remapping in remappings.txt that maps
`@openzeppelin/contracts/` to
lib/openzeppelin-contracts-upgradeable/lib/openzeppelin-contracts/contracts/ is
correct but depends on nested submodules; update the CI/checkout step so the
repo and its submodules are initialized recursively (e.g., ensure the workflow
uses git submodule update --init --recursive or the checkout action with
submodules: true and recursive submodule initialization) so the path
materializes at build time and the remapping in remappings.txt resolves
correctly.
In `@contracts/ens-label-healer/script/Grant.s.sol`:
- Around line 4-5: The build fails because imported dependency paths (e.g.,
import "forge-std/Script.sol" in Grant.s.sol) reference submodules that are
registered in .gitmodules but not checked out; run git submodule update --init
--recursive locally and ensure the CI config runs the same command before
building so directories like lib/forge-std and
lib/openzeppelin-contracts-upgradeable are populated and the imports resolve.
In `@contracts/ens-label-healer/src/ENSLabelHealer.sol`:
- Around line 23-28: The initialize function grants DEFAULT_ADMIN_ROLE to the
provided admin without checking for the zero address; add a guard at the start
of initialize to require(admin != address(0)) and revert with a clear error
(e.g., "admin-zero-address") before calling _grantRole(DEFAULT_ADMIN_ROLE,
admin), ensuring the contract is not initialized with address(0) which would
lock upgradeability and pausing; update initialize accordingly (function
initialize(address admin) ...).
In `@contracts/ens-label-healer/test/ENSLabelHealer.invariant.t.sol`:
- Around line 8-50: The handler currently tests healer.submit but not
healer.submitBatch, so add a submitBatch(string[] calldata labels) external
function to ENSLabelHealerHandler that mirrors submit: record logs with
vm.recordLogs(), vm.prank(submitter) then try healer.submitBatch(labels) and if
healer.paused() inspect vm.getRecordedLogs() for the LabelHealed(string)
signature (keccak256("LabelHealed(string)")) and set labelHealedWhilePaused =
true if found; keep the existing pause()/unpause helpers unchanged.
In `@contracts/README.md`:
- Line 9: In the contracts README entry for the ENSLabelHealer row (the table
line containing `ens-label-healer/` and `ENSLabelHealer`), replace the word
"on-chain" with the repository-preferred spelling "onchain" so the description
reads "Permissioned onchain label emitter for unresolved ENS labels"; update
only that text token in the table row.
---
Duplicate comments:
In `@contracts/ens-label-healer/.env.example`:
- Around line 1-17: The .env example uses plain assignments so child processes
(and Foundry's vm.envAddress/vm.envString lookups) won't see them; update the
variables (e.g., ADMIN_ADDRESS, SUBMITTER_ADDRESS, DEPLOYER_PRIVATE_KEY,
SEPOLIA_RPC_URL, MAINNET_RPC_URL, ETHERSCAN_API_KEY) to be exported (add the
export keyword) so that forge subprocesses inherit them when running source .env
&& forge script ...; ensure the example shows exported variables and mention any
required vs optional ones in the comments near the corresponding names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5af242e3-60c8-4118-9805-bba21b637f39
📒 Files selected for processing (15)
.github/workflows/contracts_ci.yml.gitmodulescontracts/README.mdcontracts/ens-label-healer/.env.examplecontracts/ens-label-healer/.gas-snapshotcontracts/ens-label-healer/.gitignorecontracts/ens-label-healer/README.mdcontracts/ens-label-healer/foundry.tomlcontracts/ens-label-healer/remappings.txtcontracts/ens-label-healer/script/Deploy.s.solcontracts/ens-label-healer/script/Grant.s.solcontracts/ens-label-healer/src/ENSLabelHealer.solcontracts/ens-label-healer/test/ENSLabelHealer.fuzz.t.solcontracts/ens-label-healer/test/ENSLabelHealer.invariant.t.solcontracts/ens-label-healer/test/ENSLabelHealer.t.sol
| _grantRole(DEFAULT_ADMIN_ROLE, admin); | ||
| } | ||
|
|
||
| function pause() external onlyRole(DEFAULT_ADMIN_ROLE) { |
There was a problem hiding this comment.
I like the pausability in the abstract, but I can't think of a concrete reason we'd need it in practice; wdyt?
There was a problem hiding this comment.
for example we could face an error in ENS-Rainbow-Emitter. for example it could send duplicate events. in theory we dont want to turn off ens-rainbow-emiiter to accept labels from users, but want to pause emitting events.
this is just one idea, something else could happend. adding this function would only increase gas during deploy, no runtime (emitting LabelHealed) gas increase
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (7)
contracts/ens-label-healer/script/Grant.s.sol (1)
18-26:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winZero-address guards are still missing.
SUBMITTER_ADDRESSandPROXY_ADDRESSare not validated before use. Passingaddress(0)as the submitter silently wastes gas; passingaddress(0)as the proxy produces a confusing revert. A past review marked this resolved, but the current code still lacks the guards.🛠️ Proposed fix
function run() external { address proxyAddress = vm.envAddress("PROXY_ADDRESS"); address submitter = vm.envAddress("SUBMITTER_ADDRESS"); + + require(proxyAddress != address(0), "PROXY_ADDRESS must not be zero"); + require(submitter != address(0), "SUBMITTER_ADDRESS must not be zero"); ENSLabelHealer proxy = ENSLabelHealer(proxyAddress);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/ens-label-healer/script/Grant.s.sol` around lines 18 - 26, The run() function reads PROXY_ADDRESS and SUBMITTER_ADDRESS from vm.envAddress without zero-address checks; add guards before using them: validate proxyAddress != address(0) and submitter != address(0) (e.g., require or revert with clear messages) before casting to ENSLabelHealer and before calling proxy.grantSubmitter, so you fail fast with a helpful error instead of wasting gas or producing confusing reverts; keep vm.startBroadcast()/vm.stopBroadcast() as-is and perform the proxyAddress check prior to ENSLabelHealer(proxyAddress) to avoid invalid cast.contracts/ens-label-healer/script/Deploy.s.sol (1)
19-26:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeploying with
OWNER_ADDRESS=0x0can permanently brick the proxy — add a zero-address guard.If
ownerresolves toaddress(0)(e.g., env var unset), the behavior depends on the OZ version:
- OZ v4:
__Ownable_init(address(0))silently setsowner = address(0). EveryonlyOwnercall (includingupgradeToAndCall) will revert forever — the proxy is permanently non-upgradeable and non-administrable.- OZ v5:
__Ownable_init_unchaineddoes revert withOwnableInvalidOwner, but the error surface is a cryptic mid-deployment revert rather than a clear pre-flight failure.A script-level guard catches the misconfiguration immediately with an actionable message, and mirrors the check that was applied to the other scripts.
A past review flagged this; the Vercel bot claimed the fix was applied, but it's absent from the current code.
🛠️ Proposed fix
function run() external { address owner = vm.envAddress("OWNER_ADDRESS"); + require(owner != address(0), "OWNER_ADDRESS must not be zero"); vm.startBroadcast();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/ens-label-healer/script/Deploy.s.sol` around lines 19 - 26, The run() deployment should guard against OWNER_ADDRESS being the zero address to avoid bricking the proxy: after reading owner via vm.envAddress("OWNER_ADDRESS") and before vm.startBroadcast(), add a check that owner != address(0) and revert/emit a clear error message if it is unset; update the Deploy.s.sol script around the owner variable and ENSLabelHealer/ERC1967Proxy creation to abort early with a descriptive message so the proxy is never initialized with address(0).contracts/ens-label-healer/.env.example (1)
1-16:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
source .envwon't propagate variables toforge— addexportprefix.Shell variables set by
source .envwithoutexportare shell-local and not inherited by child processes. Theforgesubprocess reads them viavm.envAddress()/vm.envString()from the OS environment, so they must be exported. Thesource .env && forge script ...pattern documented in the README will silently fail to passOWNER_ADDRESS,DEPLOYER_PRIVATE_KEY, etc. to Forge.🛠️ Proposed fix
-OWNER_ADDRESS= +export OWNER_ADDRESS= -SUBMITTER_ADDRESS= +export SUBMITTER_ADDRESS= -DEPLOYER_PRIVATE_KEY= +export DEPLOYER_PRIVATE_KEY= -SEPOLIA_RPC_URL= +export SEPOLIA_RPC_URL= -MAINNET_RPC_URL= +export MAINNET_RPC_URL= -ETHERSCAN_API_KEY= +export ETHERSCAN_API_KEY=🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/ens-label-healer/.env.example` around lines 1 - 16, The .env variables are not exported so child processes like Forge won't see them; update the .env.example by prefixing each environment variable (OWNER_ADDRESS, SUBMITTER_ADDRESS, DEPLOYER_PRIVATE_KEY, SEPOLIA_RPC_URL, MAINNET_RPC_URL, ETHERSCAN_API_KEY) with export so that sourcing the file (source .env) exports them into the environment and vm.envAddress()/vm.envString() used by Forge can read them.contracts/ens-label-healer/README.md (1)
3-5:⚠️ Potential issue | 🟡 Minor | 💤 Low valueUse "onchain" (no hyphen) per repo conventions.
Lines 3 and 5 both use "on-chain". The preferred spelling in this repository is "onchain".
📝 Proposed fix
-Permissioned on-chain label emitter for unresolved ENS labels. +Permissioned onchain label emitter for unresolved ENS labels.-`ENSLabelHealer` lets trusted submitters publish labels on-chain via `LabelHealed(string label)`. +`ENSLabelHealer` lets trusted submitters publish labels onchain via `LabelHealed(string label)`.Based on learnings: "In the namehash/ensnode repository, the correct/preferred spelling is 'onchain' (one word, no hyphen)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/ens-label-healer/README.md` around lines 3 - 5, Update README.md to use the repository-preferred spelling "onchain" instead of "on-chain" in the description of the ENSLabelHealer project; locate the lines referencing "on-chain label emitter" and any other occurrences (e.g., the sentence describing ENS registry event behavior and the ENSLabelHealer/LabelHealed mention) and replace "on-chain" with "onchain" so the README consistently uses the one-word form..gitmodules (1)
1-9: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winPin submodule versions for reproducibility.
None of the three submodule entries carry a
branchdirective or an inline version comment. While Foundry records the pinned commit SHA in the git index,forge updatewill pull the upstream default branch (master), and the chosen versions are invisible from.gitmodulesalone. Explicitly pin each submodule, either viaforge installwith a version tag (e.g.OpenZeppelin/openzeppelin-contracts-upgradeable@v5.3.0) or by adding abranch = vX.Y.Zline, so the intent is auditable and CI failures from upstream drift are preventable.🛠️ Example
[submodule "contracts/ens-label-healer/lib/openzeppelin-contracts-upgradeable"] path = contracts/ens-label-healer/lib/openzeppelin-contracts-upgradeable url = https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable + branch = v5.3.0 [submodule "contracts/ens-label-healer/lib/forge-std"] path = contracts/ens-label-healer/lib/forge-std url = https://github.com/foundry-rs/forge-std + branch = v1.9.6 [submodule "contracts/ens-label-healer/lib/openzeppelin-contracts"] path = contracts/ens-label-healer/lib/openzeppelin-contracts url = https://github.com/OpenZeppelin/openzeppelin-contracts + branch = v5.3.0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.gitmodules around lines 1 - 9, Update the three submodule entries to pin their upstream versions so builds are reproducible: for each submodule (contracts/ens-label-healer/lib/openzeppelin-contracts-upgradeable, contracts/ens-label-healer/lib/forge-std, contracts/ens-label-healer/lib/openzeppelin-contracts) either run a versioned install (e.g. forge install OpenZeppelin/openzeppelin-contracts-upgradeable@vX.Y.Z) or add a branch = vX.Y.Z line to the corresponding .gitmodules entry to record the intended release tag; ensure the chosen vX.Y.Z values match the committed SHAs in the git index so CI and other contributors can reproduce the pinned versions..github/workflows/contracts_ci.yml (1)
11-15:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStill applicable: this workflow won't rerun when CI plumbing changes.
The
pathsfilters only watchcontracts/**, so edits to this workflow or.gitmodulescan break contract CI without triggering the job.Suggested fix
push: branches: - main paths: - "contracts/**" + - ".github/workflows/contracts_ci.yml" + - ".gitmodules" pull_request: paths: - "contracts/**" + - ".github/workflows/contracts_ci.yml" + - ".gitmodules"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/contracts_ci.yml around lines 11 - 15, The workflow's paths filter currently restricts runs to "contracts/**" (the two occurrences under the top-level trigger and pull_request), which prevents reruns when CI plumbing like this workflow file or .gitmodules changes; update the paths filters to also include the workflow file and CI-relevant files (e.g., ".github/workflows/contracts_ci.yml", ".gitmodules", and any other CI config) or remove the restrictive paths filter so changes to the workflow itself will trigger the job; modify the two occurrences of the paths block in the file to reflect this change.contracts/ens-label-healer/test/ENSLabelHealer.invariant.t.sol (1)
21-49: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winStill applicable: the invariant handler never exercises
submitBatch.Right now the paused-state invariant only probes
ENSLabelHealer.submit. IfENSLabelHealer.submitBatchever lost itswhenNotPausedguard, this suite would stay green.Suggested addition
function submit(string calldata label) external { bool paused = healer.paused(); vm.recordLogs(); vm.prank(submitter); try healer.submit(label) { if (paused) { Vm.Log[] memory logs = vm.getRecordedLogs(); bytes32 sig = keccak256("LabelHealed(string)"); for (uint256 i = 0; i < logs.length; i++) { if (logs[i].topics[0] == sig) labelHealedWhilePaused = true; } } } catch {} } + + function submitBatch(string[4] calldata input, uint8 seed) external { + uint256 len = (uint256(seed) % 4) + 1; + string[] memory labels = new string[](len); + for (uint256 i = 0; i < len; i++) { + labels[i] = input[i]; + } + + bool paused = healer.paused(); + + vm.recordLogs(); + vm.prank(submitter); + try healer.submitBatch(labels) { + if (paused) { + Vm.Log[] memory logs = vm.getRecordedLogs(); + bytes32 sig = keccak256("LabelHealed(string)"); + for (uint256 i = 0; i < logs.length; i++) { + if (logs[i].topics[0] == sig) labelHealedWhilePaused = true; + } + } + } catch {} + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/ens-label-healer/test/ENSLabelHealer.invariant.t.sol` around lines 21 - 49, The invariant test only exercises healer.submit; add a call path that exercises ENSLabelHealer.submitBatch too (so batch-submission losing whenNotPaused would be caught). Modify the existing submit handler (or add a new handler) to vm.prank(submitter) and call healer.submitBatch with a single-element string[] containing the same label, wrapped in the same try/catch and vm.recordLogs() logic; mirror the paused check and the LabelHealed(string) log-scan that sets labelHealedWhilePaused so both submit and submitBatch are checked for paused enforcement.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/ens-label-healer/.env.example`:
- Around line 1-16: Add a missing PROXY_ADDRESS entry to the .env.example so
scripts that call vm.envAddress("PROXY_ADDRESS") in Grant.s.sol and Revoke.s.sol
can read it; update the template by adding a clear placeholder line for
PROXY_ADDRESS with a short comment indicating it should be set to the deployed
proxy address.
In `@contracts/ens-label-healer/lib/openzeppelin-contracts`:
- Line 1: Add documentation linking the openzeppelin-contracts submodule SHA
6dd405a07fd94e7466bb573a909445efed0c3e55 to its nearest OpenZeppelin release:
create or update the repository README (or PR notes) to state the exact commit
SHA, include a URL to the upstream OpenZeppelin commit, indicate that this SHA
is untagged, and note which official OpenZeppelin semver release (or nearest
tag) it most closely corresponds to for auditability; ensure the entry
references "openzeppelin-contracts" and the SHA string exactly so reviewers can
verify the mapping.
In `@contracts/ens-label-healer/script/Revoke.s.sol`:
- Around line 18-26: Add early non-zero address guards in run(): after reading
proxyAddress and submitter from vm.envAddress, require proxyAddress !=
address(0) with a clear message before constructing ENSLabelHealer(proxyAddress)
and require submitter != address(0) with a clear message before calling
proxy.revokeSubmitter(submitter); keep the existing
vm.startBroadcast()/vm.stopBroadcast() flow and place the checks before
vm.startBroadcast to fail fast on misconfiguration.
In `@contracts/ens-label-healer/src/ENSLabelHealer.sol`:
- Around line 18-35: The implementation contract lacks a constructor to lock
initializers; add a constructor in ENSLabelHealer that calls
_disableInitializers() so the implementation cannot be initialized directly.
Locate the ENSLabelHealer contract and add a parameterless constructor that
invokes Initializable._disableInitializers() (or simply _disableInitializers())
to prevent calls to initialize(); leave the existing initialize(),
SUBMITTER_ROLE, and inherited initializers unchanged.
---
Duplicate comments:
In @.github/workflows/contracts_ci.yml:
- Around line 11-15: The workflow's paths filter currently restricts runs to
"contracts/**" (the two occurrences under the top-level trigger and
pull_request), which prevents reruns when CI plumbing like this workflow file or
.gitmodules changes; update the paths filters to also include the workflow file
and CI-relevant files (e.g., ".github/workflows/contracts_ci.yml",
".gitmodules", and any other CI config) or remove the restrictive paths filter
so changes to the workflow itself will trigger the job; modify the two
occurrences of the paths block in the file to reflect this change.
In @.gitmodules:
- Around line 1-9: Update the three submodule entries to pin their upstream
versions so builds are reproducible: for each submodule
(contracts/ens-label-healer/lib/openzeppelin-contracts-upgradeable,
contracts/ens-label-healer/lib/forge-std,
contracts/ens-label-healer/lib/openzeppelin-contracts) either run a versioned
install (e.g. forge install
OpenZeppelin/openzeppelin-contracts-upgradeable@vX.Y.Z) or add a branch = vX.Y.Z
line to the corresponding .gitmodules entry to record the intended release tag;
ensure the chosen vX.Y.Z values match the committed SHAs in the git index so CI
and other contributors can reproduce the pinned versions.
In `@contracts/ens-label-healer/.env.example`:
- Around line 1-16: The .env variables are not exported so child processes like
Forge won't see them; update the .env.example by prefixing each environment
variable (OWNER_ADDRESS, SUBMITTER_ADDRESS, DEPLOYER_PRIVATE_KEY,
SEPOLIA_RPC_URL, MAINNET_RPC_URL, ETHERSCAN_API_KEY) with export so that
sourcing the file (source .env) exports them into the environment and
vm.envAddress()/vm.envString() used by Forge can read them.
In `@contracts/ens-label-healer/README.md`:
- Around line 3-5: Update README.md to use the repository-preferred spelling
"onchain" instead of "on-chain" in the description of the ENSLabelHealer
project; locate the lines referencing "on-chain label emitter" and any other
occurrences (e.g., the sentence describing ENS registry event behavior and the
ENSLabelHealer/LabelHealed mention) and replace "on-chain" with "onchain" so the
README consistently uses the one-word form.
In `@contracts/ens-label-healer/script/Deploy.s.sol`:
- Around line 19-26: The run() deployment should guard against OWNER_ADDRESS
being the zero address to avoid bricking the proxy: after reading owner via
vm.envAddress("OWNER_ADDRESS") and before vm.startBroadcast(), add a check that
owner != address(0) and revert/emit a clear error message if it is unset; update
the Deploy.s.sol script around the owner variable and
ENSLabelHealer/ERC1967Proxy creation to abort early with a descriptive message
so the proxy is never initialized with address(0).
In `@contracts/ens-label-healer/script/Grant.s.sol`:
- Around line 18-26: The run() function reads PROXY_ADDRESS and
SUBMITTER_ADDRESS from vm.envAddress without zero-address checks; add guards
before using them: validate proxyAddress != address(0) and submitter !=
address(0) (e.g., require or revert with clear messages) before casting to
ENSLabelHealer and before calling proxy.grantSubmitter, so you fail fast with a
helpful error instead of wasting gas or producing confusing reverts; keep
vm.startBroadcast()/vm.stopBroadcast() as-is and perform the proxyAddress check
prior to ENSLabelHealer(proxyAddress) to avoid invalid cast.
In `@contracts/ens-label-healer/test/ENSLabelHealer.invariant.t.sol`:
- Around line 21-49: The invariant test only exercises healer.submit; add a call
path that exercises ENSLabelHealer.submitBatch too (so batch-submission losing
whenNotPaused would be caught). Modify the existing submit handler (or add a new
handler) to vm.prank(submitter) and call healer.submitBatch with a
single-element string[] containing the same label, wrapped in the same try/catch
and vm.recordLogs() logic; mirror the paused check and the LabelHealed(string)
log-scan that sets labelHealedWhilePaused so both submit and submitBatch are
checked for paused enforcement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8c5222f3-2d73-41e3-b915-55bc7382c8bb
📒 Files selected for processing (16)
.github/workflows/contracts_ci.yml.gitmodulescontracts/ens-label-healer/.env.examplecontracts/ens-label-healer/LICENCEcontracts/ens-label-healer/README.mdcontracts/ens-label-healer/lib/forge-stdcontracts/ens-label-healer/lib/openzeppelin-contractscontracts/ens-label-healer/lib/openzeppelin-contracts-upgradeablecontracts/ens-label-healer/remappings.txtcontracts/ens-label-healer/script/Deploy.s.solcontracts/ens-label-healer/script/Grant.s.solcontracts/ens-label-healer/script/Revoke.s.solcontracts/ens-label-healer/src/ENSLabelHealer.solcontracts/ens-label-healer/test/ENSLabelHealer.fuzz.t.solcontracts/ens-label-healer/test/ENSLabelHealer.invariant.t.solcontracts/ens-label-healer/test/ENSLabelHealer.t.sol
lightwalker-eth
left a comment
There was a problem hiding this comment.
@sevenzing Hey thanks for the updates here. Shared feedback on next steps 👍
| @@ -0,0 +1,9 @@ | |||
| # ENSNode Contracts | |||
|
|
|||
| Solidity smart contracts used in the ENSNode ecosystem. Each contract lives in its own subdirectory as a standalone [Foundry](https://book.getfoundry.sh/) project. | |||
There was a problem hiding this comment.
| Solidity smart contracts used in the ENSNode ecosystem. Each contract lives in its own subdirectory as a standalone [Foundry](https://book.getfoundry.sh/) project. | |
| Solidity smart contracts created as an explicit part of the ENSNode platform. Each contract lives in its own subdirectory as a standalone [Foundry](https://book.getfoundry.sh/) project. | |
| Note that this is not a directory of all ENS contracts. For general directories of ENS contracts see: | |
| 1. The [datasources package in the ENSNode monorepo](https://github.com/namehash/ensnode/tree/main/packages/datasources). | |
| 2. The [ens-contracts repo](https://github.com/ensdomains/ens-contracts). | |
| 3. The [contracts-v2 repo](https://github.com/ensdomains/contracts-v2). |
|
|
||
| | Directory | Contract | Purpose | | ||
| | ------------------- | ---------------- | ------------------------------------------------------------- | | ||
| | `ens-label-healer/` | `ENSLabelHealer` | Permissioned on-chain label emitter for unresolved ENS labels | |
There was a problem hiding this comment.
| | `ens-label-healer/` | `ENSLabelHealer` | Permissioned on-chain label emitter for unresolved ENS labels | | |
| | `ens-name-healer/` | `ENSNameHealer` | Enable healed labels in ENS names to automatically propagate to all decentralized ENSNode instances | |
Feedback:
- ENSNameHealer is better branding than ENSLabelHealer. As discussed we can name this according to it's ultimately value proposition, rather than as a technical description of how it is implemented. Please apply all the related renamings.
- Updated purpose to focus more on the value proposition.
| PausableUpgradeable, | ||
| UUPSUpgradeable | ||
| { | ||
| bytes32 public constant SUBMITTER_ROLE = keccak256("SUBMITTER_ROLE"); |
There was a problem hiding this comment.
I note how the current approach here defines the submitter as a role that any number of accounts can be granted.
We don't ever want there to be more than 1 submitter though. That increases the probability of race conditions and coordination problems across submitters. Allowing more than 1 submitter is a source of problems.
The submitter should not be a role. It should be a single address.
Likewise, we should remove the idea of this contract being pausable. If we want to pause the contract for some reason, we will do this by changing the address of the sole-submitter to the null address.
We should therefore also change grantSubmitter / revokeSubmitter (and their associated scripts). The operation here should be to setSubmitter and which is what we would want scripts to help with.
| /// @title ENSLabelHealer | ||
| /// @notice Permissioned on-chain emitter for unresolved ENS labels. | ||
| /// | ||
| /// Some ENS registry contracts emit events containing only a labelhash. |
There was a problem hiding this comment.
We've previously invested in writing up docs with related info. There's an opportunity to better align what's written here with our existing docs.
| PausableUpgradeable, | ||
| UUPSUpgradeable | ||
| { | ||
| bytes32 public constant SUBMITTER_ROLE = keccak256("SUBMITTER_ROLE"); |
There was a problem hiding this comment.
We should document the why behind our key design choices and make them explicit.
For example, with regard to the single submitter design:
A single submitter may be designed at a time. This supports the goal of minimizing the number of duplicate LabelHealed events indexers need to consume. Supporting multiple submitters concurrently would create coordination complexity (ex: mitigating race conditions). Only a single instance of ENSRainbowBeam, fulfilling the role of the submitter, is needed across the whole ENS ecosystem.
| _revokeRole(SUBMITTER_ROLE, submitter); | ||
| } | ||
|
|
||
| function submit(string calldata label) external onlyRole(SUBMITTER_ROLE) whenNotPaused { |
There was a problem hiding this comment.
We should rename / refine some of our terminology to avoid confusion.
The terminology of "submitter" / "submit" will cause confusion based on how the overall solution here will work. We need to think about this contract not only in isolation by itself, but as part of a larger solution of systems and people working together.
Note how other people / other apps will be "submitting" labels to ENSRainbowBeam through pathways such as:
- An updated version of https://ensrainbow.io/ that will add a multi-line text field where anyone can go and dump a bunch of labels into and press "Heal".
- Anyone who might implement the new
EnsRainbowBeamClientthat I understand @djstrong is building in his ENSRainbowBeam PR which is a lightweight wrapper around the ENSRainbowBeam REST API. For example, the official ENS apps may start using this when they discover a label based on a user's input. - Anyone who is directly sending HTTP requests to our ENSRainbowBeam instance that we will publicly host for the whole ENS ecosystem to use.
Therefore, who is really "the submitter" here? See how it will be confusing?
To solve for this, here's some suggested terminology:
discoverer- this is the address of who discovers a label and submits it to ENSRainbowBeam. Each time a label submission is made to ENSRainbowBeam (of a single label, or many labels in bulk) that submission may optionally have adiscovererwho would then receive attribution for any of the submitted labels that are ultimately emitted onchain.- ... part of the goal here is that we will eventually build a leaderboard for who the best discoverers are. We want to recognize and reward contributions!
- Note how the
discovereris optional. Sometimes labels will be discovered but thediscovererwill be undefined. That's ok.
publisher- this is the sole and exclusive address assigned the authority to publish labels that have been discovered to the ENSNameHealer contract. In other words, I'm suggesting to rename the idea of the "submitter" as is currently implemented into this contract to be the "publisher" instead.
Another idea that we need to give special attention to here is the word label or labels. This is another key source of potential confusion. We must add formalism to explicitly document what these label values can be, because it's not what many developers might initially assume: all of the label values are what we refer to as a LiteralLabel.
A LiteralLabel is any possible string and is not constrained to the set of strings that are valid labels in ENSIP-15 normalized ENS names. Therefore, all of the following are examples of possible LiteralLabel values:
- The empty string
- A string containing label separator characters '.'
- A string containing null byte characters.
- A string containing character sequences that are not ENSIP-15 normalized or normalizable.
- Etc.
We can note how this enables even unnormalized ENS names to become healed, and how this has a benefit of enabling better classification of label between "unknown" vs "unnormalized".
Based on all of the ideas above, suggest to change the following functions:
function submit(string calldata label) external onlyRole(SUBMITTER_ROLE) whenNotPausedfunction submitBatch(string[] calldata labels) external onlyRole(SUBMITTER_ROLE) whenNotPaused
To the following instead:
function publishLabel(string calldata literalLabel) external onlyPublisherfunction publishLabel(string calldata literalLabel, address discoverer) external onlyPublisherfunction publishLabels(string[] calldata literalLabels) external onlyPublisherfunction publishLabels(string[] calldata literalLabels, address discoverer) external onlyPublisher
(please note: in the above suggestion I'm not focused on anything technical, which may need further refinement. In the suggestions above I'm exclusively focused on narrative)
We should also rename LabelHealed to LabelPublished -- this contract doesn't heal anything. When we discussed calling it "ENSNameHealer" we agreed to do that in the context of talking about it's value proposition, not its actual implementation. This contract publishes labels to all distributed ENSNode instances so that their ENSIndexer service may use published labels that are indexed to heal unknown labels (where possible).
We should also explicitly be documenting key that shape the fundamental design of how everything here works together, including this contract, the publisher, the community of discoverers, and indexers.
- The open community of unknown label discoverers use any available techniques to discover labels in indexed ENS name data that are unknown. These unknown labels are represented as encoded labelhashes in indexed ENS name data. Therefore, a discovered label represents the preimage of the encoded labelhash of an unknown label.
- When anyone in the open community of unknown label discoverers believes they discovered the preimage of the encoded labelhash of an unknown label in 1 or more indexed ENS names, that discoverer should submit the discovered labels (labelhash preimages) to the ENSRainbowBeam instance operating as the sole "publisher" of labels through the ENSNameHealer contract. Each time a discoverer submits labels to ENSRainbowBeam they may optionally attribute the discovery of those labels to their address for recognition of the discovery.
- ENSRainbowBeam has the responsibility to minimize the volume of labels published through the ENSNameHealer contract that would not result in indexers healing 1 or more already-indexed labels. In other words, to minimize unproductive work for all ENS indexers.
- The ENSNameHealer contract has the responsibility to enforce that only the designated publisher can publish labels and that all published labels are emitted to indexers.
- Indexers have the responsibility to correctly interpret each LiteralLabel emitted by the ENSNameHealer contract and to appropriately update indexed data. Indexers must gracefully handle the possibility that a published label has already been published or results in no incremental labels being healed. Indexers are suggested to store all published labels indefinitely such that a published label indexed at time T could also be used by the indexer to heal another label at time T+N.
| import "@openzeppelin/contracts-upgradeable/utils/PausableUpgradeable.sol"; | ||
|
|
||
| /// @title ENSLabelHealer | ||
| /// @notice Permissioned on-chain emitter for unresolved ENS labels. |
There was a problem hiding this comment.
Please see related feedback on how we should introduce this contract. This includes how we explicitly document its role as part of a larger set of systems, etc.
| @@ -0,0 +1,177 @@ | |||
| # ENSLabelHealer | |||
|
|
|||
| Permissioned on-chain label emitter for unresolved ENS labels. | |||
There was a problem hiding this comment.
Please see related feedback for how we document this contract, introduce its purpose, and document it's role and responsibilities within a larger set of systems.
Lite PR
Tip: Review docs on the ENSNode PR process
Summary
Why
Testing
Notes for Reviewer (Optional)
Pre-Review Checklist (Blocking)