Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KSI-189: Implement remove nodes #13102

Merged
merged 2 commits into from
May 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/tiny-rocks-shake.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"chainlink": patch
---

#internal generate geth wrappers for capability registry remove nodes
5 changes: 5 additions & 0 deletions contracts/.changeset/stupid-horses-promise.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@chainlink/contracts": patch
---

implement remove nodes on capability registry
24 changes: 24 additions & 0 deletions contracts/src/v0.8/keystone/CapabilityRegistry.sol
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,10 @@ contract CapabilityRegistry is OwnerIsCreator, TypeAndVersionInterface {
/// @param nodeOperatorId The ID of the node operator that manages this node
event NodeAdded(bytes32 p2pId, uint256 nodeOperatorId);

/// @notice This event is emitted when a node is removed
/// @param p2pId The P2P ID of the node that was removed
event NodeRemoved(bytes32 p2pId);

/// @notice This event is emitted when a node is updated
/// @param p2pId The P2P ID of the node
/// @param nodeOperatorId The ID of the node operator that manages this node
Expand Down Expand Up @@ -269,6 +273,26 @@ contract CapabilityRegistry is OwnerIsCreator, TypeAndVersionInterface {
}
}

/// @notice Removes nodes. The node operator admin or contract owner
/// can remove nodes
/// @param removedNodeP2PIds The P2P Ids of the nodes to remove
function removeNodes(bytes32[] calldata removedNodeP2PIds) external {
bool isOwner = msg.sender == owner();
for (uint256 i; i < removedNodeP2PIds.length; ++i) {
bytes32 p2pId = removedNodeP2PIds[i];
Node memory node = s_nodes[p2pId];

bool nodeExists = s_nodes[p2pId].supportedHashedCapabilityIds.length > 0;
if (!nodeExists) revert InvalidNodeP2PId(p2pId);

NodeOperator memory nodeOperator = s_nodeOperators[node.nodeOperatorId];

if (!isOwner && msg.sender != nodeOperator.admin) revert AccessForbidden();
delete s_nodes[p2pId];
emit NodeRemoved(p2pId);
Comment on lines +290 to +292
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once DON management is complete, we must validate that the node doesn't belong to any DONs. It'd be great if you could create a ticket to keep track of it.

}
}

/// @notice Updates nodes. The node admin can update the node's signer address
/// and reconfigure its supported capabilities
/// @param nodes The nodes to update
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import {BaseTest} from "./BaseTest.t.sol";
import {CapabilityRegistry} from "../CapabilityRegistry.sol";

contract CapabilityRegistry_RemoveNodesTest is BaseTest {
event NodeRemoved(bytes32 p2pId);

uint256 private constant TEST_NODE_OPERATOR_ONE_ID = 0;
uint256 private constant TEST_NODE_OPERATOR_TWO_ID = 1;
bytes32 private constant INVALID_P2P_ID = bytes32("fake-p2p");

function setUp() public override {
BaseTest.setUp();
changePrank(ADMIN);
s_capabilityRegistry.addNodeOperators(_getNodeOperators());
s_capabilityRegistry.addCapability(s_basicCapability);
s_capabilityRegistry.addCapability(s_capabilityWithConfigurationContract);

CapabilityRegistry.Node[] memory nodes = new CapabilityRegistry.Node[](1);
bytes32[] memory hashedCapabilityIds = new bytes32[](2);
hashedCapabilityIds[0] = s_basicHashedCapabilityId;
hashedCapabilityIds[1] = s_capabilityWithConfigurationContractId;

nodes[0] = CapabilityRegistry.Node({
nodeOperatorId: TEST_NODE_OPERATOR_ONE_ID,
p2pId: P2P_ID,
signer: NODE_OPERATOR_ONE_SIGNER_ADDRESS,
supportedHashedCapabilityIds: hashedCapabilityIds
});

changePrank(NODE_OPERATOR_ONE_ADMIN);

s_capabilityRegistry.addNodes(nodes);
}

function test_RevertWhen_CalledByNonNodeOperatorAdminAndNonOwner() public {
changePrank(STRANGER);
bytes32[] memory nodes = new bytes32[](1);
nodes[0] = P2P_ID;

vm.expectRevert(CapabilityRegistry.AccessForbidden.selector);
s_capabilityRegistry.removeNodes(nodes);
}

function test_RevertWhen_NodeDoesNotExist() public {
changePrank(NODE_OPERATOR_ONE_ADMIN);
bytes32[] memory nodes = new bytes32[](1);
nodes[0] = INVALID_P2P_ID;

vm.expectRevert(abi.encodeWithSelector(CapabilityRegistry.InvalidNodeP2PId.selector, INVALID_P2P_ID));
s_capabilityRegistry.removeNodes(nodes);
}

function test_RevertWhen_P2PIDEmpty() public {
changePrank(NODE_OPERATOR_ONE_ADMIN);
bytes32[] memory nodes = new bytes32[](1);
nodes[0] = bytes32("");

vm.expectRevert(abi.encodeWithSelector(CapabilityRegistry.InvalidNodeP2PId.selector, bytes32("")));
s_capabilityRegistry.removeNodes(nodes);
}

function test_RemovesNode() public {
changePrank(NODE_OPERATOR_ONE_ADMIN);

bytes32[] memory nodes = new bytes32[](1);
nodes[0] = P2P_ID;

vm.expectEmit(address(s_capabilityRegistry));
emit NodeRemoved(P2P_ID);
s_capabilityRegistry.removeNodes(nodes);

CapabilityRegistry.Node memory node = s_capabilityRegistry.getNode(P2P_ID);
assertEq(node.nodeOperatorId, 0);
assertEq(node.p2pId, bytes32(""));
assertEq(node.signer, address(0));
assertEq(node.supportedHashedCapabilityIds.length, 0);
}

function test_OwnerCanRemoveNodes() public {
changePrank(ADMIN);

bytes32[] memory nodes = new bytes32[](1);
nodes[0] = P2P_ID;

vm.expectEmit(address(s_capabilityRegistry));
emit NodeRemoved(P2P_ID);
s_capabilityRegistry.removeNodes(nodes);

CapabilityRegistry.Node memory node = s_capabilityRegistry.getNode(P2P_ID);
assertEq(node.nodeOperatorId, 0);
assertEq(node.p2pId, bytes32(""));
assertEq(node.signer, address(0));
assertEq(node.supportedHashedCapabilityIds.length, 0);
}
}

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
GETH_VERSION: 1.13.8
forwarder: ../../../contracts/solc/v0.8.19/KeystoneForwarder/KeystoneForwarder.abi ../../../contracts/solc/v0.8.19/KeystoneForwarder/KeystoneForwarder.bin b4c900aae9e022f01abbac7993d41f93912247613ac6270b0c4da4ef6f2016e3
keystone_capability_registry: ../../../contracts/solc/v0.8.19/CapabilityRegistry/CapabilityRegistry.abi ../../../contracts/solc/v0.8.19/CapabilityRegistry/CapabilityRegistry.bin 98d53a1997053a3037827ffd170c12f49d2005a5c266a1ea9eb69bb51e862f37
keystone_capability_registry: ../../../contracts/solc/v0.8.19/CapabilityRegistry/CapabilityRegistry.abi ../../../contracts/solc/v0.8.19/CapabilityRegistry/CapabilityRegistry.bin 878d2b539e07962af90e8c283fa5a90a15b5b59ddbc0854a137a3be621f0afcd
ocr3_capability: ../../../contracts/solc/v0.8.19/OCR3Capability/OCR3Capability.abi ../../../contracts/solc/v0.8.19/OCR3Capability/OCR3Capability.bin 9dcbdf55bd5729ba266148da3f17733eb592c871c2108ccca546618628fd9ad2
Loading