refactor: use lazy IMT for ciphernode registry#1387
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughDeregistration flow was simplified: sibling/IMT proof handling removed across CLI, tasks, contracts, tests, and docs; registry migrated from LeanIMT to LazyIMT with per-node index and enabled tracking; contract and ABI signatures for deregister/remove were updated to be parameterless. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant BondingRegistry
participant CiphernodeRegistry
participant LazyIMT
User->>CLI: run "enclave ciphernode deregister"
CLI->>BondingRegistry: call deregisterOperator()
BondingRegistry->>CiphernodeRegistry: removeCiphernode(msg.sender)
CiphernodeRegistry->>CiphernodeRegistry: lookup ciphernodeTreeIndex[msg.sender]
CiphernodeRegistry->>LazyIMT: removeLeafAt(index)
LazyIMT-->>CiphernodeRegistry: updated root/size
CiphernodeRegistry-->>BondingRegistry: emit CiphernodeRemoved(index, newSize)
BondingRegistry-->>CLI: tx confirmed / event
CLI-->>User: report success
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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 unit tests (beta)
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 |
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol (1)
310-319:⚠️ Potential issue | 🟠 MajorReject zero-address inserts in
addCiphernode.Line 310 currently allows
address(0). Since Line 333 uses0as the removal tombstone (_update(0, index)), allowing zero-address insertion can create ambiguous membership/accounting behavior.🛠️ Suggested fix
function addCiphernode(address node) external onlyOwnerOrBondingVault { + require(node != address(0), ZeroAddress()); if (isEnabled(node)) { return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol` around lines 310 - 319, The addCiphernode function must reject the zero address to avoid ambiguity with the tombstone value; add a require that node != address(0) (with a clear revert message) at the start of addCiphernode (before isEnabled and before using ciphernodes._insert) so you never insert uint160(0) into the tree or set ciphernodeEnabled/ciphernodeTreeIndex/numCiphernodes for address(0); this change pertains to function addCiphernode and the uses of ciphernodes._insert, ciphernodeEnabled, ciphernodeTreeIndex, and numCiphernodes.
🧹 Nitpick comments (1)
packages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.ts (1)
569-603: Consider adding a zero-address regression test in this block.Given the new tombstone-based removal semantics, add a test that
addCiphernode(0x000...000)reverts withZeroAddressto lock in boundary behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.ts` around lines 569 - 603, Add a new test in the "removeCiphernode()" describe block that asserts addCiphernode called with the zero address reverts with the custom error ZeroAddress: use the existing setup fixture (e.g., const { registry } = await loadFixture(setup)), then call registry.addCiphernode(ethers.constants.AddressZero) (or registry.addCiphernode("0x000...0")) and expect it .to.be.revertedWithCustomError(registry, "ZeroAddress"); place this alongside the other it() tests to lock in the zero-address regression for addCiphernode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/pages/ciphernode-operators/exits-and-slashing.mdx`:
- Around line 95-101: The summary tables still reference the old proof-based
deregistration; update all table entries that mention "proof-based
deregistration" or show the old command to instead reflect the new no-proof flow
by replacing the proof-based command with the new CLI invocation `enclave
ciphernode deregister` and adjusting any descriptive text to remove
proof-related steps; search for the "Step 1: Deregister" heading and any summary
table rows under it (and the nearby table blocks that previously referenced
proof-based deregistration) and change their command/examples and explanatory
text to the no-proof flow consistently.
---
Outside diff comments:
In `@packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol`:
- Around line 310-319: The addCiphernode function must reject the zero address
to avoid ambiguity with the tombstone value; add a require that node !=
address(0) (with a clear revert message) at the start of addCiphernode (before
isEnabled and before using ciphernodes._insert) so you never insert uint160(0)
into the tree or set ciphernodeEnabled/ciphernodeTreeIndex/numCiphernodes for
address(0); this change pertains to function addCiphernode and the uses of
ciphernodes._insert, ciphernodeEnabled, ciphernodeTreeIndex, and numCiphernodes.
---
Nitpick comments:
In `@packages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.ts`:
- Around line 569-603: Add a new test in the "removeCiphernode()" describe block
that asserts addCiphernode called with the zero address reverts with the custom
error ZeroAddress: use the existing setup fixture (e.g., const { registry } =
await loadFixture(setup)), then call
registry.addCiphernode(ethers.constants.AddressZero) (or
registry.addCiphernode("0x000...0")) and expect it
.to.be.revertedWithCustomError(registry, "ZeroAddress"); place this alongside
the other it() tests to lock in the zero-address regression for addCiphernode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b93c4759-e13b-4492-ae36-6e963585659c
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
crates/cli/src/ciphernode/lifecycle.rscrates/cli/src/ciphernode/mod.rscrates/cli/src/ciphernode/utils.rsdocs/pages/ciphernode-operators/exits-and-slashing.mdxpackages/enclave-contracts/contracts/interfaces/IBondingRegistry.solpackages/enclave-contracts/contracts/interfaces/ICiphernodeRegistry.solpackages/enclave-contracts/contracts/registry/BondingRegistry.solpackages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.solpackages/enclave-contracts/contracts/test/MockCiphernodeRegistry.solpackages/enclave-contracts/hardhat.config.tspackages/enclave-contracts/package.jsonpackages/enclave-contracts/tasks/ciphernode.tspackages/enclave-contracts/test/Enclave.spec.tspackages/enclave-contracts/test/Registry/BondingRegistry.spec.tspackages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.ts
💤 Files with no reviewable changes (3)
- crates/cli/src/ciphernode/utils.rs
- packages/enclave-contracts/hardhat.config.ts
- packages/enclave-contracts/test/Enclave.spec.ts
ctrlc03
reviewed
Mar 5, 2026
ctrlc03
approved these changes
Mar 5, 2026
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.
Closes #1385
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Refactor