Skip to content

Commit

Permalink
Merge pull request #13206 from smartcontractkit/KS-199/update-node-si…
Browse files Browse the repository at this point in the history 
…gner-type

KS-199:  Update node signer type
  • Loading branch information
@cds95
cds95 authored May 17, 2024
2 parents 2e698a0 + 833fd74 commit f2854c5
Show file tree
Hide file tree
Showing 9 changed files with 190 additions and 37 deletions.
5 changes: 5 additions & 0 deletions .changeset/two-papayas-begin.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"chainlink": patch
---

#internal update node signer type
5 changes: 5 additions & 0 deletions contracts/.changeset/sour-parents-explode.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@chainlink/contracts": patch
---

update node signer type
47 changes: 35 additions & 12 deletions contracts/src/v0.8/keystone/CapabilityRegistry.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ contract CapabilityRegistry is OwnerIsCreator, TypeAndVersionInterface {
/// @notice The id of the node operator that manages this node
uint32 nodeOperatorId;
/// @notice The signer address for application-layer message verification.
address signer;
bytes32 signer;
/// @notice This is an Ed25519 public key that is used to identify a node.
/// This key is guaranteed to be unique in the CapabilityRegistry. It is
/// used to identify a node in the the P2P network.
Expand All @@ -42,7 +42,13 @@ contract CapabilityRegistry is OwnerIsCreator, TypeAndVersionInterface {
/// @notice The number of times the node's capability has been updated
uint32 configCount;
/// @notice The signer address for application-layer message verification.
address signer;
/// @dev This key is guaranteed to be unique in the CapabilityRegistry
/// as a signer address can only belong to one node.
/// @dev This should be the ABI encoded version of the node's address.
/// I.e 0x0000address. The Capability Registry does not store it as an address so that
/// non EVM chains with addresses greater than 20 bytes can be supported
/// in the future.
bytes32 signer;
/// @notice This is an Ed25519 public key that is used to identify a node.
/// This key is guaranteed to be unique in the CapabilityRegistry. It is
/// used to identify a node in the the P2P network.
Expand Down Expand Up @@ -158,7 +164,8 @@ contract CapabilityRegistry is OwnerIsCreator, TypeAndVersionInterface {
/// @notice This event is emitted when a new node is added
/// @param p2pId The P2P ID of the node
/// @param nodeOperatorId The ID of the node operator that manages this node
event NodeAdded(bytes32 p2pId, uint256 nodeOperatorId);
/// @param signer The encoded node's signer address
event NodeAdded(bytes32 p2pId, uint256 nodeOperatorId, bytes32 signer);

/// @notice This event is emitted when a node is removed
/// @param p2pId The P2P ID of the node that was removed
Expand All @@ -168,7 +175,7 @@ contract CapabilityRegistry is OwnerIsCreator, TypeAndVersionInterface {
/// @param p2pId The P2P ID of the node
/// @param nodeOperatorId The ID of the node operator that manages this node
/// @param signer The node's signer address
event NodeUpdated(bytes32 p2pId, uint256 nodeOperatorId, address signer);
event NodeUpdated(bytes32 p2pId, uint256 nodeOperatorId, bytes32 signer);

/// @notice This event is emitted when a new DON is created
/// @param donId The ID of the newly created DON
Expand All @@ -184,7 +191,8 @@ contract CapabilityRegistry is OwnerIsCreator, TypeAndVersionInterface {
error DONDoesNotExist(uint32 donId);

/// @notice This error is thrown when trying to set the node's
/// signer address to zero
/// signer address to zero or if the signer address has already
/// been used by another node
error InvalidNodeSigner();

/// @notice This error is thrown when trying add a capability that already
Expand Down Expand Up @@ -259,6 +267,9 @@ contract CapabilityRegistry is OwnerIsCreator, TypeAndVersionInterface {
/// Deprecated capabilities are skipped by the `getCapabilities` function.
EnumerableSet.Bytes32Set private s_deprecatedHashedCapabilityIds;

/// @notice Encoded node signer addresses
EnumerableSet.Bytes32Set private s_nodeSigners;

/// @notice Mapping of node operators
mapping(uint256 nodeOperatorId => NodeOperator nodeOperator) private s_nodeOperators;

Expand Down Expand Up @@ -346,10 +357,10 @@ contract CapabilityRegistry is OwnerIsCreator, TypeAndVersionInterface {
NodeOperator memory nodeOperator = s_nodeOperators[node.nodeOperatorId];
if (!isOwner && msg.sender != nodeOperator.admin) revert AccessForbidden();

bool nodeExists = s_nodes[node.p2pId].signer != address(0);
bool nodeExists = s_nodes[node.p2pId].signer != bytes32("");
if (nodeExists || bytes32(node.p2pId) == bytes32("")) revert InvalidNodeP2PId(node.p2pId);

if (node.signer == address(0)) revert InvalidNodeSigner();
if (bytes32(node.signer) == bytes32("") || s_nodeSigners.contains(node.signer)) revert InvalidNodeSigner();

bytes32[] memory capabilityIds = node.hashedCapabilityIds;
if (capabilityIds.length == 0) revert InvalidNodeCapabilities(capabilityIds);
Expand All @@ -365,7 +376,8 @@ contract CapabilityRegistry is OwnerIsCreator, TypeAndVersionInterface {
s_nodes[node.p2pId].nodeOperatorId = node.nodeOperatorId;
s_nodes[node.p2pId].p2pId = node.p2pId;
s_nodes[node.p2pId].signer = node.signer;
emit NodeAdded(node.p2pId, node.nodeOperatorId);
s_nodeSigners.add(node.signer);
emit NodeAdded(node.p2pId, node.nodeOperatorId, node.signer);
}
}

Expand All @@ -377,12 +389,13 @@ contract CapabilityRegistry is OwnerIsCreator, TypeAndVersionInterface {
for (uint256 i; i < removedNodeP2PIds.length; ++i) {
bytes32 p2pId = removedNodeP2PIds[i];

bool nodeExists = s_nodes[p2pId].signer != address(0);
bool nodeExists = bytes32(s_nodes[p2pId].signer) != bytes32("");
if (!nodeExists) revert InvalidNodeP2PId(p2pId);

NodeOperator memory nodeOperator = s_nodeOperators[s_nodes[p2pId].nodeOperatorId];

if (!isOwner && msg.sender != nodeOperator.admin) revert AccessForbidden();
s_nodeSigners.remove(s_nodes[p2pId].signer);
delete s_nodes[p2pId];
emit NodeRemoved(p2pId);
}
Expand All @@ -400,10 +413,13 @@ contract CapabilityRegistry is OwnerIsCreator, TypeAndVersionInterface {
NodeOperator memory nodeOperator = s_nodeOperators[node.nodeOperatorId];
if (!isOwner && msg.sender != nodeOperator.admin) revert AccessForbidden();

bool nodeExists = s_nodes[node.p2pId].signer != address(0);
bool nodeExists = s_nodes[node.p2pId].signer != bytes32("");
if (!nodeExists) revert InvalidNodeP2PId(node.p2pId);

if (node.signer == address(0)) revert InvalidNodeSigner();
if (
bytes32(node.signer) == bytes32("") ||
(s_nodes[node.p2pId].signer != node.signer && s_nodeSigners.contains(node.signer))
) revert InvalidNodeSigner();

bytes32[] memory supportedCapabilityIds = node.hashedCapabilityIds;
if (supportedCapabilityIds.length == 0) revert InvalidNodeCapabilities(supportedCapabilityIds);
Expand All @@ -418,7 +434,14 @@ contract CapabilityRegistry is OwnerIsCreator, TypeAndVersionInterface {

s_nodes[node.p2pId].nodeOperatorId = node.nodeOperatorId;
s_nodes[node.p2pId].p2pId = node.p2pId;
s_nodes[node.p2pId].signer = node.signer;

bytes32 previousSigner = s_nodes[node.p2pId].signer;

if (s_nodes[node.p2pId].signer != node.signer) {
s_nodeSigners.remove(previousSigner);
s_nodes[node.p2pId].signer = node.signer;
s_nodeSigners.add(node.signer);
}
emit NodeUpdated(node.p2pId, node.nodeOperatorId, node.signer);
}
}
Expand Down
39 changes: 34 additions & 5 deletions contracts/src/v0.8/keystone/test/CapabilityRegistry_AddNodesTest.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ import {BaseTest} from "./BaseTest.t.sol";
import {CapabilityRegistry} from "../CapabilityRegistry.sol";

contract CapabilityRegistry_AddNodesTest is BaseTest {
event NodeAdded(bytes32 p2pId, uint256 nodeOperatorId);
event NodeAdded(bytes32 p2pId, uint256 nodeOperatorId, bytes32 signer);

uint32 private constant TEST_NODE_OPERATOR_ONE_ID = 0;
uint256 private constant TEST_NODE_OPERATOR_TWO_ID = 1;
uint32 private constant TEST_NODE_OPERATOR_TWO_ID = 1;

function setUp() public override {
BaseTest.setUp();
Expand Down Expand Up @@ -46,14 +46,43 @@ contract CapabilityRegistry_AddNodesTest is BaseTest {
nodes[0] = CapabilityRegistry.NodeParams({
nodeOperatorId: TEST_NODE_OPERATOR_ONE_ID,
p2pId: P2P_ID,
signer: address(0),
signer: bytes32(""),
hashedCapabilityIds: hashedCapabilityIds
});

vm.expectRevert(abi.encodeWithSelector(CapabilityRegistry.InvalidNodeSigner.selector));
s_capabilityRegistry.addNodes(nodes);
}

function test_RevertWhen_SignerAddressNotUnique() public {
changePrank(NODE_OPERATOR_ONE_ADMIN);
CapabilityRegistry.NodeParams[] memory nodes = new CapabilityRegistry.NodeParams[](1);

bytes32[] memory hashedCapabilityIds = new bytes32[](1);
hashedCapabilityIds[0] = s_basicHashedCapabilityId;

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

s_capabilityRegistry.addNodes(nodes);

changePrank(NODE_OPERATOR_TWO_ADMIN);

// Try adding another node with the same signer address
nodes[0] = CapabilityRegistry.NodeParams({
nodeOperatorId: TEST_NODE_OPERATOR_TWO_ID,
p2pId: P2P_ID_TWO,
signer: NODE_OPERATOR_ONE_SIGNER_ADDRESS,
hashedCapabilityIds: hashedCapabilityIds
});
vm.expectRevert(abi.encodeWithSelector(CapabilityRegistry.InvalidNodeSigner.selector));
s_capabilityRegistry.addNodes(nodes);
}

function test_RevertWhen_AddingDuplicateP2PId() public {
changePrank(NODE_OPERATOR_ONE_ADMIN);
CapabilityRegistry.NodeParams[] memory nodes = new CapabilityRegistry.NodeParams[](1);
Expand Down Expand Up @@ -143,7 +172,7 @@ contract CapabilityRegistry_AddNodesTest is BaseTest {
});

vm.expectEmit(address(s_capabilityRegistry));
emit NodeAdded(P2P_ID, TEST_NODE_OPERATOR_ONE_ID);
emit NodeAdded(P2P_ID, TEST_NODE_OPERATOR_ONE_ID, NODE_OPERATOR_ONE_SIGNER_ADDRESS);
s_capabilityRegistry.addNodes(nodes);

(CapabilityRegistry.NodeParams memory node, uint32 configCount) = s_capabilityRegistry.getNode(P2P_ID);
Expand Down Expand Up @@ -171,7 +200,7 @@ contract CapabilityRegistry_AddNodesTest is BaseTest {
});

vm.expectEmit(address(s_capabilityRegistry));
emit NodeAdded(P2P_ID, TEST_NODE_OPERATOR_ONE_ID);
emit NodeAdded(P2P_ID, TEST_NODE_OPERATOR_ONE_ID, NODE_OPERATOR_ONE_SIGNER_ADDRESS);
s_capabilityRegistry.addNodes(nodes);

(CapabilityRegistry.NodeParams memory node, uint32 configCount) = s_capabilityRegistry.getNode(P2P_ID);
Expand Down
35 changes: 33 additions & 2 deletions contracts/src/v0.8/keystone/test/CapabilityRegistry_RemoveNodesTest.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,11 +75,42 @@ contract CapabilityRegistry_RemoveNodesTest is BaseTest {
(CapabilityRegistry.NodeParams memory node, uint32 configCount) = s_capabilityRegistry.getNode(P2P_ID);
assertEq(node.nodeOperatorId, 0);
assertEq(node.p2pId, bytes32(""));
assertEq(node.signer, address(0));
assertEq(node.signer, bytes32(""));
assertEq(node.hashedCapabilityIds.length, 0);
assertEq(configCount, 0);
}

function test_CanAddNodeWithSameSignerAddressAfterRemoving() public {
changePrank(NODE_OPERATOR_ONE_ADMIN);

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

s_capabilityRegistry.removeNodes(nodes);

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

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

s_capabilityRegistry.addNodes(nodeParams);

(CapabilityRegistry.NodeParams memory node, uint32 configCount) = s_capabilityRegistry.getNode(P2P_ID);
assertEq(node.nodeOperatorId, TEST_NODE_OPERATOR_ONE_ID);
assertEq(node.p2pId, P2P_ID);
assertEq(node.hashedCapabilityIds.length, 2);
assertEq(node.hashedCapabilityIds[0], s_basicHashedCapabilityId);
assertEq(node.hashedCapabilityIds[1], s_capabilityWithConfigurationContractId);
assertEq(configCount, 1);
}

function test_OwnerCanRemoveNodes() public {
changePrank(ADMIN);

Expand All @@ -93,7 +124,7 @@ contract CapabilityRegistry_RemoveNodesTest is BaseTest {
(CapabilityRegistry.NodeParams memory node, uint32 configCount) = s_capabilityRegistry.getNode(P2P_ID);
assertEq(node.nodeOperatorId, 0);
assertEq(node.p2pId, bytes32(""));
assertEq(node.signer, address(0));
assertEq(node.signer, bytes32(""));
assertEq(node.hashedCapabilityIds.length, 0);
assertEq(configCount, 0);
}
Expand Down
Loading

0 comments on commit f2854c5

Please sign in to comment.