hoprd/rest-api/src/alias.rs (1)

253-306: Simplify error handling in get_alias_address.

The function has multiple unwrap operations that could be simplified using the ? operator.

Consider this refactor:

-    let alias = urlencoding::decode(&alias);
-
-    if alias.is_err() {
-        return (StatusCode::BAD_REQUEST, ApiErrorStatus::InvalidInput).into_response();
-    }
-
-    let alias = alias.unwrap().into_owned();
+    let alias = urlencoding::decode(&alias)
+        .map_err(|_| (StatusCode::BAD_REQUEST, ApiErrorStatus::InvalidInput))?
+        .into_owned();

sdk/python/api/hopr.py (1)

160-172: Consider simplifying the conditional logic.

The function could be more concise while maintaining readability.

Consider using a ternary operator:

-        if return_address:
-            is_ok, response = await self.__call_api(HTTPMethod.GET, "aliases_addresses")
-            return response if is_ok else None
-        else:
-            is_ok, response = await self.__call_api(HTTPMethod.GET, "aliases")
-            return response if is_ok else None
+        endpoint = "aliases_addresses" if return_address else "aliases"
+        is_ok, response = await self.__call_api(HTTPMethod.GET, endpoint)
+        return response if is_ok else None

tests/test_integration.py (1)

95-98: LGTM! Consider adding a docstring to clarify the test's extended scope.

The new assertions correctly verify the address retrieval functionality for both individual alias and aliases list endpoints. The lowercase address validation is a good practice for Ethereum address handling.

Consider adding a docstring to document that this test now verifies both PeerId and ETH address retrieval:

 async def test_hoprd_should_be_able_to_remove_existing_aliases(peer: str, swarm7: dict[str, Node]):
+    """
+    Test alias management functionality including:
+    - Setting and removing aliases
+    - Retrieving PeerIds for aliases
+    - Retrieving ETH addresses for aliases (in lowercase)
+    """

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

sdk/python/api/hopr.py (2)

160-168: Enhance type safety and optimize string usage.

A few improvements can be made to this method:

1. Add return type annotation
2. Remove unnecessary f-strings
3. Enhance docstring with return type information

-    async def aliases_get_aliases(self, return_address: bool = False):
+    async def aliases_get_aliases(self, return_address: bool = False) -> Optional[list[Union[Alias, AliasAddress]]]:
         """
         Returns all aliases.
         :param: return_address: bool. If true, returns addresses instead of peer_ids
-        :return: aliases: list
+        :return: List of Alias or AliasAddress objects, or None if the request fails
         """
-        endpoint = f"aliases_addresses" if return_address else f"aliases"
+        endpoint = "aliases_addresses" if return_address else "aliases"
         is_ok, response = await self.__call_api(HTTPMethod.GET, endpoint)
         return response if is_ok else None

🧰 Tools

🪛 Ruff (0.8.2)

166-166: f-string without any placeholders

Remove extraneous f prefix

(F541)

---

166-166: f-string without any placeholders

Remove extraneous f prefix

(F541)

---

170-179: Update docstring to reflect all parameters and return types.

The implementation looks good, but the docstring needs to be updated to include the return_address parameter and clarify the return types.

     async def aliases_get_alias(self, alias: str, return_address: bool = False) -> Optional[Union[Alias, AliasAddress]]:
         """
-        Returns the peer id recognized by the node.
-        :return: alias: Alias
+        Returns the peer id or ETH address recognized by the node.
+        :param alias: The alias to look up
+        :param return_address: If True, returns ETH address instead of peer id
+        :return: Alias or AliasAddress object, or None if not found
         """

hoprd/rest-api/src/alias.rs (4)

31-41: Struct definition looks good but consider improving documentation.

The AddressResponse struct follows the same pattern as PeerIdResponse and uses proper serialization attributes. However, consider adding documentation comments to describe the purpose and usage of this struct.

Add documentation comments:

+/// Response struct for endpoints returning ETH addresses.
+/// Contains a single field representing the ETH address in the HOPR network.
 #[serde_as]
 #[derive(Debug, Clone, Serialize, utoipa::ToSchema)]

---

117-120: Optimize memory usage by avoiding unnecessary clone.

The clone() operation on alias is unnecessary as it's being moved into the HashMap.

-                .map(|alias| (alias.alias.clone(), alias.peer_id.clone()))
+                .map(|alias| (alias.alias, alias.peer_id))

---

137-141: Improve error handling in parallel operations.

Using join_all means errors in individual futures are silently converted to error strings. Consider using try_join_all to handle errors more gracefully.

-            let aliases_addresses = futures::future::join_all(aliases_addresses_futures)
+            let aliases_addresses = futures::future::try_join_all(aliases_addresses_futures)
                 .await
+                .unwrap_or_else(|e| vec![])  // Handle errors appropriately
                 .into_iter()
                 .collect::<HashMap<_, _>>();

---

286-303: Consider reducing nesting and extracting common logic.

The nested match expressions make the code harder to follow. Consider extracting the peer resolution logic into a separate function since it's used in multiple places.

Extract the peer resolution logic:

async fn resolve_peer_address(
    entry: String,
    hopr: Arc<Hopr>,
) -> Result<Address, (StatusCode, ApiErrorStatus)> {
    let peer_or_address = PeerOrAddress::from_str(entry.as_str())
        .map_err(|_| (StatusCode::NOT_FOUND, ApiErrorStatus::AliasNotFound))?;

    let destination = HoprIdentifier::new_with(peer_or_address, hopr.peer_resolver())
        .await
        .map_err(|_| (StatusCode::BAD_REQUEST, ApiErrorStatus::PeerNotFound))?;

    Ok(destination.address)
}

Then simplify the main function:

     match state.hoprd_db.resolve_alias(alias.clone()).await {
         Ok(Some(entry)) => {
-            let peer_or_address = match PeerOrAddress::from_str(entry.as_str()) {
-                Ok(destination) => destination,
-                Err(_) => return (StatusCode::NOT_FOUND, ApiErrorStatus::AliasNotFound).into_response(),
-            };
-
-            match HoprIdentifier::new_with(peer_or_address, hopr.peer_resolver()).await {
-                Ok(destination) => (
-                    StatusCode::OK,
-                    Json(AddressResponse {
-                        address: destination.address,
-                    }),
-                )
-                    .into_response(),
-                Err(_) => (StatusCode::BAD_REQUEST, ApiErrorStatus::PeerNotFound).into_response(),
-            }
+            match resolve_peer_address(entry, hopr).await {
+                Ok(address) => (
+                    StatusCode::OK,
+                    Json(AddressResponse { address }),
+                ).into_response(),
+                Err(response) => response.into_response(),
+            }
         }

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

hoprd/rest-api/src/alias.rs (4)

94-147: Consider improving error handling and concurrency.

The implementation could be enhanced in several ways:

1. Using try_join_all instead of join_all would provide better error handling for concurrent operations
2. Error messages could be more structured instead of using string conversions

Consider applying these improvements:

-            let aliases_addresses = futures::future::join_all(aliases_addresses_futures)
+            let aliases_addresses = futures::future::try_join_all(aliases_addresses_futures)
                 .await
+                .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, ApiErrorStatus::from(e)))?
                 .into_iter()
                 .collect::<HashMap<_, _>>();

-                        Err(_) => return (alias, ApiErrorStatus::PeerNotFound.to_string()),
+                        Err(e) => return Err(ApiErrorStatus::from(e)),

---

117-121: Consider optimizing database operations.

The code duplicates the alias retrieval logic from the aliases function. Consider extracting this common functionality into a shared helper function to improve maintainability.

Example refactor:

async fn get_alias_map(db: &impl HoprdDbAliasesOperations) -> Result<HashMap<String, String>, DbError> {
    let aliases = db.get_aliases().await?;
    Ok(aliases
        .into_iter()
        .map(|alias| (alias.alias.clone(), alias.peer_id.clone()))
        .collect())
}

---

288-291: Improve error handling consistency.

The error handling differs between the PeerOrAddress::from_str error (returns AliasNotFound) and the HoprIdentifier::new_with error (returns PeerNotFound). Consider using consistent error types for similar failure cases.

-                Err(_) => return (StatusCode::NOT_FOUND, ApiErrorStatus::AliasNotFound).into_response(),
+                Err(_) => return (StatusCode::BAD_REQUEST, ApiErrorStatus::PeerNotFound).into_response(),

Also applies to: 293-302

---

94-94: Maintain consistent TODO comment format.

The deprecation comments should follow a consistent format. Consider using the same format as the existing functions:

-// TODO(deprecated, will be removed in v3.0) Get each previously set alias and its corresponding ETH address as a hashmap.
+/// (deprecated, will be removed in v3.0) Get each previously set alias and its corresponding ETH address as a hashmap.

Also applies to: 254-254

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro