KS-306: Return hashed capability ids (#13453)

* return hashed capability ids

* update error message when node does not exist

* update wrappers

.changeset/cool-turtles-think.md

@@ -0,0 +1,5 @@
+---
+"chainlink": patch
+---
+
+#internal return hashed capability ids

.changeset/moody-dogs-begin.md

@@ -0,0 +1,5 @@
+---
+"chainlink": patch
+---
+
+#internal update error message when node does not exist

contracts/.changeset/long-jars-protect.md

@@ -0,0 +1,5 @@
+---
+'@chainlink/contracts': patch
+---
+
+update error message when node does not exist

contracts/.changeset/swift-ads-stare.md

@@ -0,0 +1,5 @@
+---
+'@chainlink/contracts': patch
+---
+
+return hashed capability ids

contracts/gas-snapshots/keystone.gas-snapshot

@@ -15,37 +15,37 @@ CapabilityRegistry_AddDONTest:test_RevertWhen_NodeDoesNotSupportCapability() (ga
 CapabilityRegistry_AddNodeOperatorsTest:test_AddNodeOperators() (gas: 184267)
 CapabilityRegistry_AddNodeOperatorsTest:test_RevertWhen_CalledByNonAdmin() (gas: 17624)
 CapabilityRegistry_AddNodeOperatorsTest:test_RevertWhen_NodeOperatorAdminAddressZero() (gas: 18520)
-CapabilityRegistry_AddNodesTest:test_AddsNodeInfo() (gas: 355458)
-CapabilityRegistry_AddNodesTest:test_OwnerCanAddNodes() (gas: 355446)
-CapabilityRegistry_AddNodesTest:test_RevertWhen_AddingDuplicateP2PId() (gas: 301431)
-CapabilityRegistry_AddNodesTest:test_RevertWhen_AddingNodeWithInvalidCapability() (gas: 55243)
+CapabilityRegistry_AddNodesTest:test_AddsNodeInfo() (gas: 355444)
+CapabilityRegistry_AddNodesTest:test_OwnerCanAddNodes() (gas: 355432)
+CapabilityRegistry_AddNodesTest:test_RevertWhen_AddingDuplicateP2PId() (gas: 301394)
+CapabilityRegistry_AddNodesTest:test_RevertWhen_AddingNodeWithInvalidCapability() (gas: 55229)
 CapabilityRegistry_AddNodesTest:test_RevertWhen_AddingNodeWithInvalidNodeOperator() (gas: 24962)
-CapabilityRegistry_AddNodesTest:test_RevertWhen_AddingNodeWithoutCapabilities() (gas: 27738)
+CapabilityRegistry_AddNodesTest:test_RevertWhen_AddingNodeWithoutCapabilities() (gas: 27724)
 CapabilityRegistry_AddNodesTest:test_RevertWhen_CalledByNonNodeOperatorAdminAndNonOwner() (gas: 25130)
-CapabilityRegistry_AddNodesTest:test_RevertWhen_P2PIDEmpty() (gas: 27444)
-CapabilityRegistry_AddNodesTest:test_RevertWhen_SignerAddressEmpty() (gas: 27083)
-CapabilityRegistry_AddNodesTest:test_RevertWhen_SignerAddressNotUnique() (gas: 309850)
+CapabilityRegistry_AddNodesTest:test_RevertWhen_P2PIDEmpty() (gas: 27430)
+CapabilityRegistry_AddNodesTest:test_RevertWhen_SignerAddressEmpty() (gas: 27069)
+CapabilityRegistry_AddNodesTest:test_RevertWhen_SignerAddressNotUnique() (gas: 309822)
 CapabilityRegistry_DeprecateCapabilitiesTest:test_DeprecatesCapability() (gas: 92873)
 CapabilityRegistry_DeprecateCapabilitiesTest:test_EmitsEvent() (gas: 93001)
 CapabilityRegistry_DeprecateCapabilitiesTest:test_RevertWhen_CalledByNonAdmin() (gas: 22879)
 CapabilityRegistry_DeprecateCapabilitiesTest:test_RevertWhen_CapabilityDoesNotExist() (gas: 16166)
 CapabilityRegistry_DeprecateCapabilitiesTest:test_RevertWhen_CapabilityIsDeprecated() (gas: 94265)
-CapabilityRegistry_GetCapabilitiesTest:test_ExcludesDeprecatedCapabilities() (gas: 119306)
-CapabilityRegistry_GetCapabilitiesTest:test_ReturnsCapabilities() (gas: 54034)
+CapabilityRegistry_GetCapabilitiesTest:test_ExcludesDeprecatedCapabilities() (gas: 122065)
+CapabilityRegistry_GetCapabilitiesTest:test_ReturnsCapabilities() (gas: 58327)
 CapabilityRegistry_GetDONsTest:test_CorrectlyFetchesDONs() (gas: 65704)
 CapabilityRegistry_GetDONsTest:test_DoesNotIncludeRemovedDONs() (gas: 88473)
 CapabilityRegistry_GetHashedCapabilityTest:test_CorrectlyGeneratesHashedCapabilityId() (gas: 11428)
 CapabilityRegistry_GetHashedCapabilityTest:test_DoesNotCauseIncorrectClashes() (gas: 13087)
 CapabilityRegistry_GetNodeOperatorsTest:test_CorrectlyFetchesNodeOperators() (gas: 36675)
 CapabilityRegistry_GetNodeOperatorsTest:test_DoesNotIncludeRemovedNodeOperators() (gas: 38982)
-CapabilityRegistry_GetNodesTest:test_CorrectlyFetchesNodes() (gas: 59880)
-CapabilityRegistry_GetNodesTest:test_DoesNotIncludeRemovedNodes() (gas: 71576)
+CapabilityRegistry_GetNodesTest:test_CorrectlyFetchesNodes() (gas: 59861)
+CapabilityRegistry_GetNodesTest:test_DoesNotIncludeRemovedNodes() (gas: 71561)
 CapabilityRegistry_RemoveDONsTest:test_RemovesDON() (gas: 60696)
 CapabilityRegistry_RemoveDONsTest:test_RevertWhen_CalledByNonAdmin() (gas: 15669)
 CapabilityRegistry_RemoveDONsTest:test_RevertWhen_DONDoesNotExist() (gas: 16540)
 CapabilityRegistry_RemoveNodeOperatorsTest:test_RemovesNodeOperator() (gas: 36069)
 CapabilityRegistry_RemoveNodeOperatorsTest:test_RevertWhen_CalledByNonOwner() (gas: 15838)
-CapabilityRegistry_RemoveNodesTest:test_CanAddNodeWithSameSignerAddressAfterRemoving() (gas: 114067)
+CapabilityRegistry_RemoveNodesTest:test_CanAddNodeWithSameSignerAddressAfterRemoving() (gas: 114053)
 CapabilityRegistry_RemoveNodesTest:test_CanRemoveWhenDONDeleted() (gas: 371521)
 CapabilityRegistry_RemoveNodesTest:test_CanRemoveWhenNodeNoLongerPartOfDON() (gas: 712537)
 CapabilityRegistry_RemoveNodesTest:test_OwnerCanRemoveNodes() (gas: 72324)

contracts/src/v0.8/keystone/CapabilityRegistry.sol

@@ -215,7 +215,7 @@ contract CapabilityRegistry is OwnerIsCreator, TypeAndVersionInterface {
   error InvalidNodeOperatorAdmin();
 
   /// @notice This error is thrown when trying to add a node with P2P ID that
-  /// is empty bytes or a duplicate.
+  /// is empty bytes
   /// @param p2pId The provided P2P ID
   error InvalidNodeP2PId(bytes32 p2pId);
 
@@ -233,12 +233,17 @@ contract CapabilityRegistry is OwnerIsCreator, TypeAndVersionInterface {
   /// been used by another node
   error InvalidNodeSigner();
 
-  /// @notice This error is thrown when trying add a capability that already
+  /// @notice This error is thrown when trying to add a capability that already
   /// exists.
   /// @param hashedCapabilityId The hashed capability ID of the capability
   /// that already exists
   error CapabilityAlreadyExists(bytes32 hashedCapabilityId);
 
+  /// @notice This error is thrown when trying to add a node that already
+  /// exists.
+  /// @param nodeP2PId The P2P ID of the node that already exists
+  error NodeAlreadyExists(bytes32 nodeP2PId);
+
   /// @notice This error is thrown when trying to add a node to a DON where
   /// the node does not support the capability
   /// @param nodeP2PId The P2P ID of the node
@@ -272,6 +277,11 @@ contract CapabilityRegistry is OwnerIsCreator, TypeAndVersionInterface {
   /// @param hashedCapabilityId The hashed ID of the capability that is deprecated.
   error CapabilityIsDeprecated(bytes32 hashedCapabilityId);
 
+  /// @notice This error is thrown when a node with the provided P2P ID is
+  /// not found.
+  /// @param nodeP2PId The node P2P ID used for the lookup.
+  error NodeDoesNotExist(bytes32 nodeP2PId);
+
   /// @notice This error is thrown when a node operator does not exist
   /// @param nodeOperatorId The ID of the node operator that does not exist
   error NodeOperatorDoesNotExist(uint32 nodeOperatorId);
@@ -469,9 +479,10 @@ contract CapabilityRegistry is OwnerIsCreator, TypeAndVersionInterface {
       if (!isOwner && msg.sender != nodeOperator.admin) revert AccessForbidden(msg.sender);
 
       Node storage storedNode = s_nodes[node.p2pId];
-      if (storedNode.signer != bytes32("") || bytes32(node.p2pId) == bytes32("")) revert InvalidNodeP2PId(node.p2pId);
+      if (storedNode.signer != bytes32("")) revert NodeAlreadyExists(node.p2pId);
+      if (node.p2pId == bytes32("")) revert InvalidNodeP2PId(node.p2pId);
 
-      if (bytes32(node.signer) == bytes32("") || s_nodeSigners.contains(node.signer)) revert InvalidNodeSigner();
+      if (node.signer == bytes32("") || s_nodeSigners.contains(node.signer)) revert InvalidNodeSigner();
 
       bytes32[] memory capabilityIds = node.hashedCapabilityIds;
       if (capabilityIds.length == 0) revert InvalidNodeCapabilities(capabilityIds);
@@ -503,7 +514,7 @@ contract CapabilityRegistry is OwnerIsCreator, TypeAndVersionInterface {
 
       Node storage node = s_nodes[p2pId];
 
-      if (bytes32(node.signer) == bytes32("")) revert InvalidNodeP2PId(p2pId);
+      if (node.signer == bytes32("")) revert NodeDoesNotExist(p2pId);
       if (node.supportedDONIds.length() > 0) revert NodePartOfDON(p2pId);
 
       if (!isOwner && msg.sender != s_nodeOperators[node.nodeOperatorId].admin) revert AccessForbidden(msg.sender);
@@ -526,11 +537,10 @@ contract CapabilityRegistry is OwnerIsCreator, TypeAndVersionInterface {
       if (!isOwner && msg.sender != nodeOperator.admin) revert AccessForbidden(msg.sender);
 
       Node storage storedNode = s_nodes[node.p2pId];
-      if (storedNode.signer == bytes32("")) revert InvalidNodeP2PId(node.p2pId);
+      if (storedNode.signer == bytes32("")) revert NodeDoesNotExist(node.p2pId);
 
-      if (
-        bytes32(node.signer) == bytes32("") || (storedNode.signer != node.signer && s_nodeSigners.contains(node.signer))
-      ) revert InvalidNodeSigner();
+      if (node.signer == bytes32("") || (storedNode.signer != node.signer && s_nodeSigners.contains(node.signer)))
+        revert InvalidNodeSigner();
 
       bytes32[] memory supportedHashedCapabilityIds = node.hashedCapabilityIds;
       if (supportedHashedCapabilityIds.length == 0) revert InvalidNodeCapabilities(supportedHashedCapabilityIds);
@@ -623,15 +633,17 @@ contract CapabilityRegistry is OwnerIsCreator, TypeAndVersionInterface {
   /// @notice Returns all capabilities. This operation will copy capabilities
   /// to memory, which can be quite expensive. This is designed to mostly be
   /// used by view accessors that are queried without any gas fees.
-  /// @return Capability[] An array of capabilities
-  function getCapabilities() external view returns (Capability[] memory) {
+  /// @return bytes32[] List of hashed capability Ids
+  /// @return Capability[] List of capabilities
+  function getCapabilities() external view returns (bytes32[] memory, Capability[] memory) {
     bytes32[] memory hashedCapabilityIds = s_hashedCapabilityIds.values();
 
+    uint256 numSupportedCapabilities = hashedCapabilityIds.length - s_deprecatedHashedCapabilityIds.length();
+
     // Solidity does not support dynamic arrays in memory, so we create a
     // fixed-size array and copy the capabilities into it.
-    Capability[] memory capabilities = new Capability[](
-      hashedCapabilityIds.length - s_deprecatedHashedCapabilityIds.length()
-    );
+    Capability[] memory capabilities = new Capability[](numSupportedCapabilities);
+    bytes32[] memory supportedHashedCapabilityIds = new bytes32[](numSupportedCapabilities);
 
     // We need to keep track of the new index because we are skipping
     // deprecated capabilities.
@@ -642,11 +654,12 @@ contract CapabilityRegistry is OwnerIsCreator, TypeAndVersionInterface {
 
       if (!s_deprecatedHashedCapabilityIds.contains(hashedCapabilityId)) {
         capabilities[newIndex] = s_capabilities[hashedCapabilityId];
-        newIndex++;
+        supportedHashedCapabilityIds[newIndex] = hashedCapabilityId;
+        ++newIndex;
       }
     }
 
-    return capabilities;
+    return (supportedHashedCapabilityIds, capabilities);
   }
 
   /// @notice This functions returns a capability id that has been hashed to fit into a bytes32 for cheaper access

contracts/src/v0.8/keystone/test/BaseTest.t.sol

@@ -46,7 +46,7 @@ contract BaseTest is Test, Constants {
     s_nonExistentHashedCapabilityId = s_capabilityRegistry.getHashedCapabilityId("non-existent-capability", "1.0.0");
   }
 
-  function _getNodeOperators() internal view returns (CapabilityRegistry.NodeOperator[] memory) {
+  function _getNodeOperators() internal pure returns (CapabilityRegistry.NodeOperator[] memory) {
     CapabilityRegistry.NodeOperator[] memory nodeOperators = new CapabilityRegistry.NodeOperator[](3);
     nodeOperators[0] = CapabilityRegistry.NodeOperator({admin: NODE_OPERATOR_ONE_ADMIN, name: NODE_OPERATOR_ONE_NAME});
     nodeOperators[1] = CapabilityRegistry.NodeOperator({admin: NODE_OPERATOR_TWO_ADMIN, name: NODE_OPERATOR_TWO_NAME});

contracts/src/v0.8/keystone/test/CapabilityRegistry_AddNodesTest.t.sol

@@ -121,7 +121,7 @@ contract CapabilityRegistry_AddNodesTest is BaseTest {
 
     s_capabilityRegistry.addNodes(nodes);
 
-    vm.expectRevert(abi.encodeWithSelector(CapabilityRegistry.InvalidNodeP2PId.selector, P2P_ID));
+    vm.expectRevert(abi.encodeWithSelector(CapabilityRegistry.NodeAlreadyExists.selector, P2P_ID));
     s_capabilityRegistry.addNodes(nodes);
   }
 

contracts/src/v0.8/keystone/test/CapabilityRegistry_GetCapabilitiesTest.t.sol

@@ -14,7 +14,12 @@ contract CapabilityRegistry_GetCapabilitiesTest is BaseTest {
   }
 
   function test_ReturnsCapabilities() public view {
-    CapabilityRegistry.Capability[] memory capabilities = s_capabilityRegistry.getCapabilities();
+    (bytes32[] memory hashedCapabilityIds, CapabilityRegistry.Capability[] memory capabilities) = s_capabilityRegistry
+      .getCapabilities();
+
+    assertEq(hashedCapabilityIds.length, 2);
+    assertEq(hashedCapabilityIds[0], keccak256(abi.encode(capabilities[0].labelledName, capabilities[0].version)));
+    assertEq(hashedCapabilityIds[1], keccak256(abi.encode(capabilities[1].labelledName, capabilities[1].version)));
 
     assertEq(capabilities.length, 2);
 
@@ -43,7 +48,12 @@ contract CapabilityRegistry_GetCapabilitiesTest is BaseTest {
     deprecatedCapabilities[0] = hashedCapabilityId;
     s_capabilityRegistry.deprecateCapabilities(deprecatedCapabilities);
 
-    CapabilityRegistry.Capability[] memory capabilities = s_capabilityRegistry.getCapabilities();
+    (bytes32[] memory hashedCapabilityIds, CapabilityRegistry.Capability[] memory capabilities) = s_capabilityRegistry
+      .getCapabilities();
+
+    assertEq(hashedCapabilityIds.length, 1);
+    assertEq(hashedCapabilityIds[0], keccak256(abi.encode(capabilities[0].labelledName, capabilities[0].version)));
+
     assertEq(capabilities.length, 1);
 
     assertEq(capabilities[0].labelledName, "read-ethereum-mainnet-gas-price");

contracts/src/v0.8/keystone/test/CapabilityRegistry_RemoveNodesTest.t.sol

@@ -62,7 +62,7 @@ contract CapabilityRegistry_RemoveNodesTest is BaseTest {
     bytes32[] memory nodes = new bytes32[](1);
     nodes[0] = INVALID_P2P_ID;
 
-    vm.expectRevert(abi.encodeWithSelector(CapabilityRegistry.InvalidNodeP2PId.selector, INVALID_P2P_ID));
+    vm.expectRevert(abi.encodeWithSelector(CapabilityRegistry.NodeDoesNotExist.selector, INVALID_P2P_ID));
     s_capabilityRegistry.removeNodes(nodes);
   }
 
@@ -71,7 +71,7 @@ contract CapabilityRegistry_RemoveNodesTest is BaseTest {
     bytes32[] memory nodes = new bytes32[](1);
     nodes[0] = bytes32("");
 
-    vm.expectRevert(abi.encodeWithSelector(CapabilityRegistry.InvalidNodeP2PId.selector, bytes32("")));
+    vm.expectRevert(abi.encodeWithSelector(CapabilityRegistry.NodeDoesNotExist.selector, bytes32("")));
     s_capabilityRegistry.removeNodes(nodes);
   }
 

contracts/src/v0.8/keystone/test/CapabilityRegistry_UpdateNodesTest.t.sol

@@ -75,7 +75,7 @@ contract CapabilityRegistry_UpdateNodesTest is BaseTest {
       hashedCapabilityIds: hashedCapabilityIds
     });
 
-    vm.expectRevert(abi.encodeWithSelector(CapabilityRegistry.InvalidNodeP2PId.selector, INVALID_P2P_ID));
+    vm.expectRevert(abi.encodeWithSelector(CapabilityRegistry.NodeDoesNotExist.selector, INVALID_P2P_ID));
     s_capabilityRegistry.updateNodes(nodes);
   }
 
@@ -93,7 +93,7 @@ contract CapabilityRegistry_UpdateNodesTest is BaseTest {
       hashedCapabilityIds: hashedCapabilityIds
     });
 
-    vm.expectRevert(abi.encodeWithSelector(CapabilityRegistry.InvalidNodeP2PId.selector, bytes32("")));
+    vm.expectRevert(abi.encodeWithSelector(CapabilityRegistry.NodeDoesNotExist.selector, bytes32("")));
     s_capabilityRegistry.updateNodes(nodes);
   }
 

core/gethwrappers/keystone/generation/generated-wrapper-dependency-versions-do-not-edit.txt

@@ -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 ec6e94293700d400ca7b22989d54793e905d6febce3b84054727a58c473b9cf3
-keystone_capability_registry: ../../../contracts/solc/v0.8.19/CapabilityRegistry/CapabilityRegistry.abi ../../../contracts/solc/v0.8.19/CapabilityRegistry/CapabilityRegistry.bin be7e699bfab89cb95e8d1318dfc74c2af9465d7347478792d6d4afd06ac85402
+keystone_capability_registry: ../../../contracts/solc/v0.8.19/CapabilityRegistry/CapabilityRegistry.abi ../../../contracts/solc/v0.8.19/CapabilityRegistry/CapabilityRegistry.bin 2913d0d38f6a6edc6d437e16d042c7a6732171213c096259f2113170550e925e
 ocr3_capability: ../../../contracts/solc/v0.8.19/OCR3Capability/OCR3Capability.abi ../../../contracts/solc/v0.8.19/OCR3Capability/OCR3Capability.bin 9dcbdf55bd5729ba266148da3f17733eb592c871c2108ccca546618628fd9ad2