XMTP Node Registry enhancements (#572)

#550 was closed in favor of this PR,
where the changes will be implemented incrementally to support the
migrator tool and further modifications.

Closes #542

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **New Features**
  - Introduced a new deployment command for version 2 of nodes.
- Enhanced node management with distinct handling for API and
replication nodes.

- **Refactor**
- Redesigned node operations with improved naming conventions and
streamlined error/event reporting.

- **Tests**
- Expanded testing coverage to include new node functionalities and
deployment scenarios.

- **Chores**
  - Updated maintainer contact information in Dockerfiles.
- Updated deployment utilities and helper configurations to support the
new NodesV2 features.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

contracts/dev/deploy-local

@@ -30,3 +30,4 @@ forge_test_contracts
 forge_deploy_script group_messages
 forge_deploy_script identity_updates
 forge_deploy_script nodes src/Nodes.sol Nodes
+forge_deploy_script nodes_v2

contracts/dev/lib/common

@@ -37,8 +37,19 @@ function forge_deploy_script() {
             echo -e "\033[32m✔\033[0m Nodes deployed. Deployment details in contracts/config/anvil_localnet/$3.json\n"
             ;;
 
+        nodes_v2)
+            echo "⧖ Deploying NodesV2 to chainId ${chain_id} using RPC ${RPC_URL}"
+            forge script --quiet --rpc-url "${RPC_URL}" --broadcast script/DeployNodeRegistryV2.s.sol || BUILD_FAILED=true
+            if [[ -n "${BUILD_FAILED:-}" ]]; then
+                echo "Failed to deploy NodesV2 contract"
+                exit 1
+            fi
+
+            echo -e "\033[32m✔\033[0m NodesV2 deployed. Deployment details in contracts/config/anvil_localnet/XMTPNodeRegistry.json\n"
+            ;;
+
         *)
-            echo "Invalid option. Use 'group_messages', 'identity_updates' or 'nodes'."
+            echo "Invalid option. Use 'group_messages', 'identity_updates', 'nodes', or 'nodes_v2'."
             exit 1
             ;;
     esac

contracts/src/NodesV2.sol

@@ -6,7 +6,6 @@ import "@openzeppelin/contracts/access/extensions/AccessControlDefaultAdminRules
 import "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
 import "./interfaces/INodes.sol";
 
-
  /// @title XMTP Nodes Registry.
  /// @notice This contract is responsible for minting NFTs and assigning them to node operators.
  /// Each node is minted as an NFT with a unique ID (starting at 100 and increasing by 100 with each new node).
@@ -46,8 +45,11 @@ contract NodesV2 is AccessControlDefaultAdminRules, ERC721, INodes {
     /// @dev Mapping of token ID to Node.
     mapping(uint256 => Node) private _nodes;
 
-    /// @dev Active Node Operators IDs set.
-    EnumerableSet.UintSet private _activeNodes;
+    /// @dev Nodes with API enabled.
+    EnumerableSet.UintSet private _activeApiNodes;
+
+    /// @dev Nodes with replication enabled.
+    EnumerableSet.UintSet private _activeReplicationNodes;
 
     /// @notice The commission percentage that the node operator receives.
     /// @dev This is stored in basis points (1/100th of a percent).
@@ -64,10 +66,16 @@ contract NodesV2 is AccessControlDefaultAdminRules, ERC721, INodes {
 
         _setRoleAdmin(ADMIN_ROLE, DEFAULT_ADMIN_ROLE);
         _setRoleAdmin(NODE_MANAGER_ROLE, DEFAULT_ADMIN_ROLE);
+        // slither-disable-next-line unused-return
         _grantRole(ADMIN_ROLE, _initialAdmin);
+        // slither-disable-next-line unused-return
         _grantRole(NODE_MANAGER_ROLE, _initialAdmin);
     }
 
+    // ***************************************************************
+    // *                ADMIN-ONLY FUNCTIONS                         *
+    // ***************************************************************
+
     /// @inheritdoc INodes
     function addNode(address to, bytes calldata signingKeyPub, string calldata httpAddress, uint256 minMonthlyFee)
         external
@@ -87,76 +95,51 @@ contract NodesV2 is AccessControlDefaultAdminRules, ERC721, INodes {
         return nodeId;
     }
 
-    /// @notice Transfers node ownership from one address to another
-    /// @dev Only the contract owner may call this. Automatically deactivates the node
-    /// @param from The current owner address
-    /// @param to The new owner address
-    /// @param nodeId The ID of the node being transferred
-    function transferFrom(address from, address to, uint256 nodeId) 
-        public 
-        override(ERC721, IERC721) 
-        onlyRole(NODE_MANAGER_ROLE)
-    {
-        _deactivateNode(nodeId);
-        super.transferFrom(from, to, nodeId);
-        emit NodeTransferred(nodeId, from, to);
-    }
-
     /// @inheritdoc INodes
-    function updateHttpAddress(uint256 nodeId, string calldata httpAddress) external onlyRole(NODE_MANAGER_ROLE) {
+    function disableNode(uint256 nodeId) public onlyRole(ADMIN_ROLE) {
         require(_nodeExists(nodeId), NodeDoesNotExist());
-        require(bytes(httpAddress).length > 0, InvalidHttpAddress());
-        _nodes[nodeId].httpAddress = httpAddress;
-        emit HttpAddressUpdated(nodeId, httpAddress);
+
+        _nodes[nodeId].isDisabled = true;
+
+        // Always remove from active nodes sets when disabled
+        _disableApiNode(nodeId);
+        _disableReplicationNode(nodeId);
+
+        emit NodeDisabled(nodeId);
     }
 
-    /// @inheritdoc INodes
-    function updateIsReplicationEnabled(uint256 nodeId, bool isReplicationEnabled) external onlyRole(NODE_MANAGER_ROLE) {
+    function removeFromApiNodes(uint256 nodeId) external onlyRole(ADMIN_ROLE) {
         require(_nodeExists(nodeId), NodeDoesNotExist());
-        _nodes[nodeId].isReplicationEnabled = isReplicationEnabled;
-        emit ReplicationEnabledUpdated(nodeId, isReplicationEnabled);
+        _disableApiNode(nodeId);
     }
 
-    /// @inheritdoc INodes
-    function updateMinMonthlyFee(uint256 nodeId, uint256 minMonthlyFee) external onlyRole(NODE_MANAGER_ROLE) {
+    function removeFromReplicationNodes(uint256 nodeId) external onlyRole(ADMIN_ROLE) {
         require(_nodeExists(nodeId), NodeDoesNotExist());
-        _nodes[nodeId].minMonthlyFee = minMonthlyFee;
-        emit MinMonthlyFeeUpdated(nodeId, minMonthlyFee);
+        _disableReplicationNode(nodeId);
     }
 
     /// @inheritdoc INodes
-    function updateActive(uint256 nodeId, bool isActive) public onlyRole(ADMIN_ROLE) {
+    function enableNode(uint256 nodeId) external onlyRole(ADMIN_ROLE) {
         require(_nodeExists(nodeId), NodeDoesNotExist());
-        if (isActive) {
-            require(_activeNodes.length() < maxActiveNodes, MaxActiveNodesReached());
-            require(_activeNodes.add(nodeId), NodeAlreadyActive());
-        } else {
-            require(_activeNodes.remove(nodeId), NodeAlreadyInactive());
-        }
-        _nodes[nodeId].isActive = isActive;
-        emit NodeActivateUpdated(nodeId, isActive);
-    }
 
-    /// @inheritdoc INodes
-    function batchUpdateActive(uint256[] calldata nodeIds, bool[] calldata isActive) 
-        external 
-        onlyRole(ADMIN_ROLE) 
-    {
-        require(nodeIds.length == isActive.length, InvalidInputLength());
-        for (uint256 i = 0; i < nodeIds.length; i++) {
-            updateActive(nodeIds[i], isActive[i]);
-        }
+        // Re-enabling a node just removes the disabled flag.
+        // The rest of the node properties are managed by the node operator.
+        _nodes[nodeId].isDisabled = false;
+
+        emit NodeEnabled(nodeId);
     }
 
     /// @inheritdoc INodes
-    function updateMaxActiveNodes(uint8 newMaxActiveNodes) external onlyRole(ADMIN_ROLE) {
-        require(newMaxActiveNodes > _activeNodes.length(), MaxActiveNodesBelowCurrentCount());
+    function setMaxActiveNodes(uint8 newMaxActiveNodes) external onlyRole(ADMIN_ROLE) {
+        if (newMaxActiveNodes < _activeApiNodes.length() || newMaxActiveNodes < _activeReplicationNodes.length()) {
+            revert MaxActiveNodesBelowCurrentCount();
+        }
         maxActiveNodes = newMaxActiveNodes;
         emit MaxActiveNodesUpdated(newMaxActiveNodes);
     }
 
     /// @inheritdoc INodes
-    function updateNodeOperatorCommissionPercent(uint256 newCommissionPercent) external onlyRole(ADMIN_ROLE) {
+    function setNodeOperatorCommissionPercent(uint256 newCommissionPercent) external onlyRole(ADMIN_ROLE) {
         require(newCommissionPercent <= MAX_BPS, InvalidCommissionPercent());
         nodeOperatorCommissionPercent = newCommissionPercent;
         emit NodeOperatorCommissionPercentUpdated(newCommissionPercent);
@@ -170,23 +153,77 @@ contract NodesV2 is AccessControlDefaultAdminRules, ERC721, INodes {
         emit BaseURIUpdated(newBaseURI);
     }
 
+    // ***************************************************************
+    // *                NODE MANAGER FUNCTIONS                       *
+    // ***************************************************************
+
+    /// @notice Transfers node ownership from one address to another
+    /// @dev Only the contract owner may call this. Automatically deactivates the node
+    /// @param from The current owner address
+    /// @param to The new owner address
+    /// @param nodeId The ID of the node being transferred
+    function transferFrom(address from, address to, uint256 nodeId) 
+        public 
+        override(ERC721, IERC721) 
+        onlyRole(NODE_MANAGER_ROLE)
+    {
+        /// @dev Disable the node before transferring ownership.
+        /// It's NOP responsibility to re-enable the node after transfer.
+        _disableApiNode(nodeId);
+        _disableReplicationNode(nodeId);
+        super.transferFrom(from, to, nodeId);
+        emit NodeTransferred(nodeId, from, to);
+    }
+
+    /// @inheritdoc INodes
+    function setHttpAddress(uint256 nodeId, string calldata httpAddress) external onlyRole(NODE_MANAGER_ROLE) {
+        require(_nodeExists(nodeId), NodeDoesNotExist());
+        require(bytes(httpAddress).length > 0, InvalidHttpAddress());
+        _nodes[nodeId].httpAddress = httpAddress;
+        emit HttpAddressUpdated(nodeId, httpAddress);
+    }
+
+    /// @inheritdoc INodes
+    function setMinMonthlyFee(uint256 nodeId, uint256 minMonthlyFee) external onlyRole(NODE_MANAGER_ROLE) {
+        require(_nodeExists(nodeId), NodeDoesNotExist());
+        _nodes[nodeId].minMonthlyFee = minMonthlyFee;
+        emit MinMonthlyFeeUpdated(nodeId, minMonthlyFee);
+    }
+
+    // ***************************************************************
+    // *                NODE OWNER FUNCTION                          *
+    // ***************************************************************
+
     /// @inheritdoc INodes
-    function updateIsApiEnabled(uint256 nodeId) external {
+    function setIsApiEnabled(uint256 nodeId, bool isApiEnabled) external whenNotDisabled(nodeId) {
         require(_ownerOf(nodeId) == msg.sender, Unauthorized());
-        _nodes[nodeId].isApiEnabled = !_nodes[nodeId].isApiEnabled;
-        emit ApiEnabledUpdated(nodeId, _nodes[nodeId].isApiEnabled);
+        if (isApiEnabled) {
+            _activateApiNode(nodeId);
+        } else {
+            _disableApiNode(nodeId);
+        }
     }
 
     /// @inheritdoc INodes
-    function getAllNodes() public view returns (NodeWithId[] memory allNodesList) {
-        allNodesList = new NodeWithId[](_nodeCounter);
+    function setIsReplicationEnabled(uint256 nodeId, bool isReplicationEnabled) external whenNotDisabled(nodeId) {
+        require(_ownerOf(nodeId) == msg.sender, Unauthorized());
+        if (isReplicationEnabled) {
+            _activateReplicationNode(nodeId);
+        } else {
+            _disableReplicationNode(nodeId);
+        }
+    }
+
+    /// @inheritdoc INodes
+    function getAllNodes() public view returns (NodeWithId[] memory allNodes) {
+        allNodes = new NodeWithId[](_nodeCounter);
         for (uint32 i = 0; i < _nodeCounter; i++) {
             uint32 nodeId = NODE_INCREMENT * (i + 1);
             if (_nodeExists(nodeId)) {
-                allNodesList[i] = NodeWithId({nodeId: nodeId, node: _nodes[nodeId]});
+                allNodes[i] = NodeWithId({nodeId: nodeId, node: _nodes[nodeId]});
             }
         }
-        return allNodesList;
+        return allNodes;
     }
 
     /// @inheritdoc INodes
@@ -201,27 +238,69 @@ contract NodesV2 is AccessControlDefaultAdminRules, ERC721, INodes {
     }
 
     /// @inheritdoc INodes
-    function getActiveNodes() external view returns (Node[] memory activeNodes) {
-        activeNodes = new Node[](_activeNodes.length());
-        for (uint32 i = 0; i < _activeNodes.length(); i++) {
-            activeNodes[i] = _nodes[_activeNodes.at(i)];
+    function getActiveApiNodes() external view returns (NodeWithId[] memory activeNodes) {
+        activeNodes = new NodeWithId[](_activeApiNodes.length());
+        for (uint32 i = 0; i < _activeApiNodes.length(); i++) {
+            uint256 nodeId = _activeApiNodes.at(i);
+            if (_nodeExists(nodeId)) {
+                activeNodes[i] = NodeWithId({nodeId: nodeId, node: _nodes[nodeId]});
+            }
+        }
+        return activeNodes;
+    }
+
+    /// @inheritdoc INodes
+    function getActiveApiNodesIDs() external view returns (uint256[] memory activeNodesIDs) {
+        return _activeApiNodes.values();
+    }
+
+    /// @inheritdoc INodes
+    function getActiveApiNodesCount() external view returns (uint256 activeNodesCount) {
+        return _activeApiNodes.length();
+    }
+
+    /// @inheritdoc INodes
+    function getApiNodeIsActive(uint256 nodeId) external view returns (bool isActive) {
+        return _activeApiNodes.contains(nodeId);
+    }
+
+    /// @inheritdoc INodes
+    function getActiveReplicationNodes() external view returns (NodeWithId[] memory activeNodes) {
+        activeNodes = new NodeWithId[](_activeReplicationNodes.length());
+        for (uint32 i = 0; i < _activeReplicationNodes.length(); i++) {
+            uint256 nodeId = _activeReplicationNodes.at(i);
+            if (_nodeExists(nodeId)) {
+                activeNodes[i] = NodeWithId({nodeId: nodeId, node: _nodes[nodeId]});
+            }
         }
         return activeNodes;
     }
 
     /// @inheritdoc INodes
-    function getActiveNodesIDs() external view returns (uint256[] memory activeNodesIDs) {
-        return _activeNodes.values();
+    function getActiveReplicationNodesIDs() external view returns (uint256[] memory activeNodesIDs) {
+        return _activeReplicationNodes.values();
     }
 
     /// @inheritdoc INodes
-    function getActiveNodesCount() external view returns (uint256 activeNodesCount) {
-        return _activeNodes.length();
+    function getActiveReplicationNodesCount() external view returns (uint256 activeNodesCount) {
+        return _activeReplicationNodes.length();
     }
 
     /// @inheritdoc INodes
-    function getNodeIsActive(uint256 nodeId) external view returns (bool isActive) {
-        return _activeNodes.contains(nodeId);
+    function getReplicationNodeIsActive(uint256 nodeId) external view returns (bool isActive) {
+        return _activeReplicationNodes.contains(nodeId);
+    }
+
+    /// @inheritdoc INodes
+    function getNodeOperatorCommissionPercent() external view returns (uint256 commissionPercent) {
+        return nodeOperatorCommissionPercent;
+    }
+
+    /// @dev Modifier to check if a node is not disabled.
+    modifier whenNotDisabled(uint256 nodeId) {
+        require(_nodeExists(nodeId), NodeDoesNotExist());
+        require(!_nodes[nodeId].isDisabled, NodeIsDisabled());
+        _;
     }
 
     /// @dev Checks if a node exists.
@@ -236,15 +315,48 @@ contract NodesV2 is AccessControlDefaultAdminRules, ERC721, INodes {
         return _baseTokenURI;
     }
 
-    /// @dev Helper function to deactivate a node
-    function _deactivateNode(uint256 nodeId) private {
-        if (_activeNodes.contains(nodeId)) {
+    /// @dev Helper function to add a node to the active API nodes set.
+    function _activateApiNode(uint256 nodeId) private {
+        require(_activeApiNodes.length() < maxActiveNodes, MaxActiveNodesReached());
+        _nodes[nodeId].isApiEnabled = true;
+        if (!_activeApiNodes.contains(nodeId)) {
+            // slither-disable-next-line unused-return
+            _activeApiNodes.add(nodeId);
+        }
+        emit ApiEnabled(nodeId);
+    }
+
+    /// @dev Helper function to remove a node from the active API nodes set.
+    function _disableApiNode(uint256 nodeId) private {
+        _nodes[nodeId].isApiEnabled = false;
+        if (_activeApiNodes.contains(nodeId)) {
+            // slither-disable-next-line unused-return
+            _activeApiNodes.remove(nodeId);
+        }
+        emit ApiDisabled(nodeId);
+    }
+
+    /// @dev Helper function to add a node to the active replication nodes set.
+    function _activateReplicationNode(uint256 nodeId) private {
+        require(_activeReplicationNodes.length() < maxActiveNodes, MaxActiveNodesReached());
+        _nodes[nodeId].isReplicationEnabled = true;
+        if (!_activeReplicationNodes.contains(nodeId)) {
+            // slither-disable-next-line unused-return
+            _activeReplicationNodes.add(nodeId);
+        }
+        emit ReplicationEnabled(nodeId);
+    }
+
+    /// @dev Helper function to remove a node from the active replication nodes set.
+    function _disableReplicationNode(uint256 nodeId) private {
+        _nodes[nodeId].isReplicationEnabled = false;
+        if (_activeReplicationNodes.contains(nodeId)) {
             // slither-disable-next-line unused-return
-            _activeNodes.remove(nodeId);
-            _nodes[nodeId].isActive = false;
-            emit NodeActivateUpdated(nodeId, false);
+            _activeReplicationNodes.remove(nodeId);
         }
+        emit ReplicationDisabled(nodeId);
     }
+
 
     /// @dev Override to support ERC721, IERC165, and AccessControlEnumerable.
     function supportsInterface(bytes4 interfaceId)