feat(contracts): add batch node status update for config managers#4886
feat(contracts): add batch node status update for config managers#4886bas-vk wants to merge 2 commits into
Conversation
bas-vk
requested review from
giuseppecrj,
jterzis,
pfives and
shuhuiluo
as code owners
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a batch API to update node statuses (configuration-manager-only), refactors per-node status update into an internal helper, updates deployment facet selectors to include the new method, and introduces tests for batch behavior, permissions, transitions, and atomicity. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/contracts/src/river/registry/facets/node/NodeRegistry.sol`:
- Around line 85-91: The config-manager path updateNodesStatusConfigManager
calls _updateNodeStatus without ensuring the target was ever registered,
allowing writes for uninitialized addresses because
_checkNodeStatusTransitionAllowed treats NotInitialized as permissive; update
_updateNodeStatus to assert the node exists before proceeding (the same
existence guard enforced by onlyNode in updateNodeStatus), e.g., require a
registration flag or that current node data is initialized, then proceed to call
_checkNodeStatusTransitionAllowed and emit NodeStatusUpdated as before to
prevent writing status for non-existent nodes.
🧹 Nitpick comments (3)
packages/contracts/src/river/registry/facets/node/INodeRegistry.sol (1)
26-44: Missing NatSpec on new struct and function.The coding guidelines require NatSpec for all public/external functions. While most existing functions in this interface also lack it, consider adding
@noticefor the new batch function to document the configuration manager restriction and batch semantics.packages/contracts/src/river/registry/facets/node/NodeRegistry.sol (1)
93-98: NatSpec missing on_updateNodeStatusandupdateNodesStatusConfigManager.Per coding guidelines, NatSpec (
@notice,@param) is expected. Brief docs clarifying the lack of existence checks vs. reliance on callers would be especially valuable here.packages/contracts/test/river/registry/node/NodeRegistry.t.sol (1)
440-628: Good coverage overall. Batch, single, empty, auth, invalid transitions, and atomicity are all tested.One gap: add a test for batch-updating a non-existent node address. This exercises the missing existence check flagged in
NodeRegistry.sol. Without it, a config manager could silently write to phantom storage slots.
Add updateNodesStatusConfigManager function to allow configuration managers to update multiple node statuses in a single transaction. Extract shared logic into _updateNodeStatus internal function used by both updateNodeStatus and updateNodesStatusConfigManager. The internal function includes a node existence check to prevent writing status for non-existent nodes. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
bas-vk
force-pushed
the
feat/batch-node-status-update
branch
from
901335b to
985ac9f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/contracts/src/river/registry/facets/node/NodeRegistry.sol (1)
85-91: Batch function looks solid — add NatSpec.Logic, access control, gas patterns (
calldata,++i) all check out. However, bothupdateNodesStatusConfigManagerand_updateNodeStatusare missing NatSpec documentation, which is required per coding guidelines.Suggested NatSpec
+ /// `@notice` Batch update node statuses by a configuration manager. + /// `@param` updates Array of node address and target status pairs. function updateNodesStatusConfigManager( UpdateNodeStatusConfigManagerRequest[] calldata updates ) external onlyConfigurationManager(msg.sender) {As per coding guidelines: "Include NatSpec documentation for all public and external functions" and "Use NatSpec documentation format with
@noticefor brief descriptions,@devfor implementation details,@paramfor parameters, and@returnfor return values".packages/contracts/test/river/registry/node/NodeRegistry.t.sol (1)
511-519: Empty array test — consider asserting no events emitted.The test confirms no revert, but doesn't assert that zero
NodeStatusUpdatedevents were emitted. Minor, since the loop body simply won't execute, butvm.recordLogs()+ length check would make the intent explicit.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/contracts/test/river/registry/node/NodeRegistry.t.sol (3)
431-438: Consider adding fuzz-input collision guards.The modifier doesn't guard against
configManagercolliding withnodeOperator,node1,node2, ordeployer. While unlikely to cause failures here, addingvm.assume(configManager != nodeOperator)(etc.) in the individual test functions would make the fuzz space more intentional and avoid accidental overlap of roles.
597-646: Solid atomicity test. One optional addition: assertnode2status is stillOperationalafter the revert for symmetry with thenode1check. The EVM revert guarantees it, but it documents intent.Optional: assert node2 unchanged too
// Verify node1 status was NOT updated (transaction reverted) Node memory nodeData1 = nodeRegistry.getNode(node1); assertEq(uint256(nodeData1.status), uint256(NodeStatus.NotInitialized)); + + // Verify node2 status was NOT changed either + Node memory nodeData2 = nodeRegistry.getNode(node2); + assertEq(uint256(nodeData2.status), uint256(NodeStatus.Operational));
426-646: Consider adding a test for duplicate node addresses within a single batch.What happens if the same node appears twice in the
updatesarray (e.g.,NotInitialized → OperationalthenOperational → Departing)? This is a valid edge case — sequential application within a batch could either succeed (chained transitions) or fail depending on implementation. Worth codifying the expected behavior.
|
Found 1 test failure on Blacksmith runners: Failure
|
4 participants
Summary
updateNodesStatusConfigManagerfunction to allow configuration managers to update multiple node statuses in a single transaction_updateNodeStatusinternal function used by bothupdateNodeStatusandupdateNodesStatusConfigManagerTest plan
🤖 Generated with Claude Code