Open
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Contributor
📝 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 |
Contributor
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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
shrugs
reviewed
May 5, 2026
Member
lightwalker-eth
left a comment
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 👍
Contributor
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
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
shrugs
approved these changes
May 7, 2026
| _grantRole(DEFAULT_ADMIN_ROLE, admin); | ||
| } | ||
|
|
||
| function pause() external onlyRole(DEFAULT_ADMIN_ROLE) { |
Member
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?
Member
Author
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
sevenzing
changed the title
Contributor
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Lite PR
Tip: Review docs on the ENSNode PR process
Summary
Why
Testing
Notes for Reviewer (Optional)
Pre-Review Checklist (Blocking)