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

feat(multichain): enhance search api #1151

Merged
merged 8 commits into from
Dec 18, 2024
Merged

Conversation

lok52
Copy link
Contributor

@lok52 lok52 commented Dec 12, 2024

  • Integrated Dapp search into quick search. In this case aggregator just proxies search requests directly to Dapp Marketplace service and then formats the response.
  • Added string prefix search for addresses. This one also supports pagination via /api/v1/addresses. In quick search it yields only top 100 matches by default.

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced a new endpoint to list addresses via the API.
    • Added support for decentralized applications (DApps) with a new client and associated data structures.
    • Enhanced search functionality to include DApps in results.
    • Implemented pagination for address queries.
    • Added a new field for marketplace DApps in search results.
    • Updated configuration to specify DApp client settings.
  • Bug Fixes

    • Improved error handling in search queries.
  • Documentation

    • Updated API specifications to reflect new features and endpoints.
    • Revised environment variable documentation for clarity.
  • Chores

    • Added new dependencies to support enhanced functionalities.

Copy link

coderabbitai bot commented Dec 12, 2024

Walkthrough

The recent changes to the multichain-aggregator project involve multiple updates across various files. The Cargo.toml files for both the multichain-aggregator and multichain-aggregator-logic packages have been updated to include a new dependency on the url crate version 2.4, and the version of the blockscout-chains dependency has been modified from 0.1.0 to 0.2.0. The justfile has been adjusted to utilize the dotenvy tool for managing environment variables during the server run command.

New structs and methods have been introduced in dapp_client.rs, including the DappClient struct and methods for searching DApps. A new module for DApps has been added to the types, and the ChainSearchResult struct now includes a vector of MarketplaceDapp objects. The API has been extended with a new endpoint, /api/v1/addresses, allowing clients to list addresses. The multichain-aggregator.proto file has been updated with new RPC methods and message types to support this functionality. Additionally, the server settings have been enhanced to include service-specific configurations, improving the overall extensibility and functionality of the project.

Poem

🐇 In the land of code where bunnies play,
New tools and structs hop in today!
With Dapps to find and addresses to list,
Our multichain dreams now can't be missed.
So let’s cheer for the changes, oh what a delight,
In the world of code, everything feels just right! 🐇✨


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 7

🧹 Outside diff range and nitpick comments (11)
multichain-aggregator/justfile (1)

30-30: Consider making the env file path configurable

While using dotenvy for environment management is a good practice, consider making the env file path configurable to support different environments and development workflows.

-    dotenvy -f multichain-aggregator-server/config/base.env cargo run --bin multichain-aggregator-server
+    dotenvy -f ${ENV_FILE:-multichain-aggregator-server/config/base.env} cargo run --bin multichain-aggregator-server
multichain-aggregator/multichain-aggregator-logic/src/types/dapp.rs (2)

4-11: Add documentation for the public struct.

Since MarketplaceDapp is a public API type, it should include documentation explaining its purpose and field meanings.

Add documentation like this:

 #[derive(Debug)]
+/// Represents a decentralized application (DApp) in the marketplace.
+///
+/// This struct contains essential metadata about a DApp including its
+/// identifier, title, logo URL, description, and associated blockchain.
 pub struct MarketplaceDapp {
+    /// Unique identifier of the DApp
     pub id: String,
+    /// Display title of the DApp
     pub title: String,
+    /// URL to the DApp's logo image
     pub logo: String,
+    /// Brief description of the DApp's functionality
     pub short_description: String,
+    /// Blockchain identifier where the DApp operates
     pub chain_id: ChainId,
 }

13-25: Consider adding input validation.

The TryFrom implementation should validate that required string fields are not empty to ensure data integrity.

Consider adding validation like this:

     fn try_from(v: DappWithChainId) -> Result<Self, Self::Error> {
+        if v.dapp.id.is_empty() {
+            return Err(ParseError::InvalidInput("DApp ID cannot be empty".into()));
+        }
+        if v.dapp.title.is_empty() {
+            return Err(ParseError::InvalidInput("DApp title cannot be empty".into()));
+        }
         Ok(Self {
             id: v.dapp.id,
             title: v.dapp.title,
multichain-aggregator/multichain-aggregator-logic/src/dapp_client.rs (2)

5-24: Add documentation for public API

The public structs and their fields should be documented to improve API clarity. Consider adding doc comments explaining:

  • The purpose of DappClient
  • The structure of Dapp and its fields
  • The relationship between Dapp and DappWithChainId

Example:

+/// Client for interacting with the DApp marketplace API
 pub struct DappClient {
     http: reqwest::Client,
     url: Url,
 }

+/// Represents a decentralized application in the marketplace
 #[derive(Debug, Deserialize)]
 #[serde(rename_all = "camelCase")]
 pub struct Dapp {
+    /// Unique identifier of the DApp
     pub id: String,
     // ... add docs for other fields
 }

32-45: Enhance error handling and input validation

The search implementation could be improved in several ways:

  1. Add input validation for the query parameter
  2. Consider implementing retry logic for failed requests
  3. Make the API path configurable
+const DEFAULT_SEARCH_PATH: &str = "/api/v1/marketplace/dapps:search";
+
 pub async fn search_dapps(&self, query: &str) -> Result<Vec<DappWithChainId>, ServiceError> {
+    if query.trim().is_empty() {
+        return Ok(Vec::new());
+    }
+
     let mut url = self.url.clone();
-    url.set_path("/api/v1/marketplace/dapps:search");
+    url.set_path(DEFAULT_SEARCH_PATH);
     url.query_pairs_mut().append_pair("query", query);

-    self.http
-        .get(url)
-        .send()
-        .await
-        .map_err(|e| ServiceError::Internal(e.into()))?
-        .json::<Vec<DappWithChainId>>()
-        .await
-        .map_err(|e| ServiceError::Internal(e.into()))
+    let response = self.http
+        .get(url.clone())
+        .send()
+        .await
+        .map_err(|e| ServiceError::Internal(format!("Failed to send request to {}: {}", url, e)))?;
+
+    if !response.status().is_success() {
+        return Err(ServiceError::Internal(format!(
+            "DApp search failed with status {}", 
+            response.status()
+        )));
+    }
+
+    response
+        .json::<Vec<DappWithChainId>>()
+        .await
+        .map_err(|e| ServiceError::Internal(format!("Failed to parse DApp response: {}", e)))
multichain-aggregator/Cargo.toml (1)

65-65: Update url crate version and add required features

Consider updating to the latest stable version and adding the serde feature if JSON serialization is needed:

-url = { version = "2.4" }
+url = { version = "2.5", features = ["serde"] }
multichain-aggregator/multichain-aggregator-server/src/settings.rs (1)

25-31: Add documentation for ServiceSettings struct

Consider adding documentation comments to describe the purpose of ServiceSettings and its fields, especially max_page_size's impact on API responses.

Add documentation like:

+/// Settings for the multichain aggregator service
+///
+/// # Fields
+/// * `dapp_client` - Configuration for the DApp marketplace client
+/// * `max_page_size` - Maximum number of items returned per page in paginated responses
 #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
 #[serde(deny_unknown_fields)]
 pub struct ServiceSettings {
multichain-aggregator/multichain-aggregator-proto/proto/v1/multichain-aggregator.proto (1)

10-11: Add RPC method documentation

Consider adding documentation comments to describe the ListAddresses RPC method's behavior and pagination.

Add documentation like:

+  // ListAddresses returns a paginated list of addresses matching the search query.
+  // The response includes both exact and prefix matches, limited by page_size.
   rpc ListAddresses(ListAddressesRequest) returns (ListAddressesResponse) {}
multichain-aggregator/multichain-aggregator-logic/src/search.rs (1)

66-72: Consider pre-allocating vector capacity for dapps

The Vec for dapps is created without an initial capacity. If you know the approximate size of dapps, pre-allocating could improve performance.

Consider this optimization:

     if let Ok(dapps) = dapps {
-        let dapps: Vec<MarketplaceDapp> = dapps
+        let dapps: Vec<MarketplaceDapp> = Vec::with_capacity(dapps.len());
+        let dapps = dapps
             .into_iter()
             .filter_map(|d| d.try_into().ok())
             .collect();
multichain-aggregator/multichain-aggregator-proto/swagger/v1/multichain-aggregator.swagger.yaml (2)

13-40: Add parameter documentation and constraints.

While the endpoint structure is good, consider enhancing the specification with:

  1. Parameter descriptions explaining:
    • q: Search query format and minimum length
    • page_size: Valid range and default value
    • page_token: Format and usage
  2. Add parameter constraints:
    page_size:
      minimum: 1
      maximum: 100
      default: 10

252-280: Enhance type definitions with descriptions and examples.

Consider adding field descriptions and examples to improve API documentation:

v1MarketplaceDapp:
  type: object
  description: Represents a decentralized application in the marketplace
  properties:
    id:
      type: string
      description: Unique identifier of the Dapp
      example: "dapp-123"
    title:
      type: string
      description: Display name of the Dapp
      example: "CryptoKitties"
    # ... similar for other fields

v1Pagination:
  type: object
  description: Standard pagination control and metadata
  properties:
    page_token:
      type: string
      description: Opaque token for fetching the next page
      example: "eyJwYWdlIjogMn0="
    # ... similar for other fields
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e03d401 and 3a62e6a.

⛔ Files ignored due to path filters (1)
  • multichain-aggregator/Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (17)
  • multichain-aggregator/Cargo.toml (1 hunks)
  • multichain-aggregator/justfile (1 hunks)
  • multichain-aggregator/multichain-aggregator-logic/Cargo.toml (1 hunks)
  • multichain-aggregator/multichain-aggregator-logic/src/dapp_client.rs (1 hunks)
  • multichain-aggregator/multichain-aggregator-logic/src/lib.rs (2 hunks)
  • multichain-aggregator/multichain-aggregator-logic/src/repository/addresses.rs (4 hunks)
  • multichain-aggregator/multichain-aggregator-logic/src/search.rs (2 hunks)
  • multichain-aggregator/multichain-aggregator-logic/src/types/dapp.rs (1 hunks)
  • multichain-aggregator/multichain-aggregator-logic/src/types/mod.rs (1 hunks)
  • multichain-aggregator/multichain-aggregator-logic/src/types/search_results.rs (3 hunks)
  • multichain-aggregator/multichain-aggregator-proto/proto/v1/api_config_http.yaml (1 hunks)
  • multichain-aggregator/multichain-aggregator-proto/proto/v1/multichain-aggregator.proto (3 hunks)
  • multichain-aggregator/multichain-aggregator-proto/swagger/v1/multichain-aggregator.swagger.yaml (3 hunks)
  • multichain-aggregator/multichain-aggregator-server/Cargo.toml (2 hunks)
  • multichain-aggregator/multichain-aggregator-server/src/server.rs (2 hunks)
  • multichain-aggregator/multichain-aggregator-server/src/services/multichain_aggregator.rs (2 hunks)
  • multichain-aggregator/multichain-aggregator-server/src/settings.rs (3 hunks)
✅ Files skipped from review due to trivial changes (1)
  • multichain-aggregator/multichain-aggregator-logic/src/types/mod.rs
🔇 Additional comments (13)
multichain-aggregator/multichain-aggregator-server/src/server.rs (2)

13-13: LGTM: Import addition is well-organized

The new DappClient import is correctly grouped with related imports from the multichain_aggregator_logic module.


74-79: Consider validating max_page_size

The max_page_size parameter should be validated to ensure it's within reasonable bounds.

Let's verify the current usage of max_page_size:

Consider adding validation before construction:

+    if settings.service.max_page_size == 0 || settings.service.max_page_size > 1000 {
+        return Err(anyhow::anyhow!(
+            "max_page_size must be between 1 and 1000, got {}",
+            settings.service.max_page_size
+        ));
+    }
     let multichain_aggregator = Arc::new(MultichainAggregator::new(
         db,
         blockscout_chains,
         dapp_client,
         settings.service.max_page_size,
     ));
multichain-aggregator/justfile (1)

30-30: Verify the environment configuration for Dapp integration

Since this PR introduces Dapp search functionality, we should verify that the base.env file contains the necessary configuration for the Dapp Marketplace service integration.

multichain-aggregator/multichain-aggregator-logic/src/repository/addresses.rs (1)

87-91: Verify safe parameterization in custom SQL expressions

When using Expr::cust_with_expr, it's crucial to ensure that input parameters are correctly parameterized to prevent SQL injection. Verify that the formatted query parameter is safely injected and that the ORM handles parameter binding securely.

multichain-aggregator/multichain-aggregator-server/src/services/multichain_aggregator.rs (2)

77-106: Well-implemented pagination in list_addresses method

The addition of the list_addresses method with proper pagination handling is well-executed. The method correctly parses the page_token, normalizes the page_size, and constructs the response with appropriate pagination details.


114-120: Updated quick_search method to integrate DappClient

The integration of DappClient into the quick_search method is correctly implemented, enhancing the search functionality to include DApp searches as intended.

multichain-aggregator/multichain-aggregator-logic/src/lib.rs (2)

2-2: Addition of dapp_client module

Including the dapp_client module extends the logic layer's capabilities to interact with the DApp marketplace, which aligns with the enhancement objectives.


11-13: Exporting ChainId for broader accessibility

Adding ChainId to the public exports allows other modules to utilize this type, promoting consistency and reuse across the codebase.

multichain-aggregator/multichain-aggregator-proto/proto/v1/api_config_http.yaml (1)

14-16: Addition of ListAddresses HTTP endpoint

The new HTTP rule correctly maps the ListAddresses RPC method to the /api/v1/addresses endpoint, enabling clients to retrieve address listings via GET requests.

multichain-aggregator/multichain-aggregator-server/Cargo.toml (1)

12-12: LGTM! Dependencies are properly configured.

The changes appropriately:

  • Add alloy-primitives for blockchain primitives handling
  • Enable the serde feature for the url crate, which is necessary for URL serialization in API responses

Also applies to: 25-25

multichain-aggregator/multichain-aggregator-logic/src/types/dapp.rs (1)

27-36: Verify chain_id omission in proto conversion.

The From<MarketplaceDapp> implementation excludes the chain_id field when converting to proto type. Please verify this is intentional and aligns with the proto definition.

Let's check the proto definition:

✅ Verification successful

The chain_id omission in proto conversion is correct

The proto definition of MarketplaceDapp in multichain-aggregator.proto does not include a chain_id field, so the current From implementation correctly excludes it when converting from the Rust struct to proto message. The Rust struct has an additional chain_id field for internal use, but it's intentionally not part of the protobuf interface.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check proto definition for MarketplaceDapp
rg -A 10 "message\s+MarketplaceDapp" **/multichain-aggregator.proto

Length of output: 142


Script:

#!/bin/bash
# Let's first find the proto file that defines MarketplaceDapp
fd -e proto | xargs rg -l "MarketplaceDapp"

Length of output: 130


Script:

#!/bin/bash
# Now let's check the MarketplaceDapp message definition
rg -A 10 "message\s+MarketplaceDapp" multichain-aggregator/multichain-aggregator-proto/proto/v1/multichain-aggregator.proto

Length of output: 368


Script:

#!/bin/bash
# Let's check the Rust struct definition to compare
rg -A 10 "struct MarketplaceDapp" multichain-aggregator/multichain-aggregator-logic/src/types/dapp.rs

Length of output: 344

multichain-aggregator/multichain-aggregator-logic/src/types/search_results.rs (1)

15-15: LGTM! Clean integration of DApp search results

The changes follow the existing pattern and maintain consistency with the codebase structure.

Also applies to: 26-26

multichain-aggregator/multichain-aggregator-proto/swagger/v1/multichain-aggregator.swagger.yaml (1)

161-165: LGTM! Clean integration of Dapp search results.

The addition of the dapps field to QuickSearchResponseChainSearchResult is well-structured and maintains backward compatibility.

@lok52 lok52 force-pushed the lok52/multichain-search-api branch from 3a62e6a to e28c105 Compare December 12, 2024 13:11
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (2)
multichain-aggregator/multichain-aggregator-server/src/services/multichain_aggregator.rs (1)

136-148: Optimize string splitting in parse_query_2 function

The parse_query_2 function converts the input string into a Vec<&str> and then immediately converts it into a slice for pattern matching. This can be optimized by splitting the string once and directly collecting the iterator into a slice without allocating a Vec, reducing memory usage and improving performance.

You can apply the following change to optimize the code:

-fn parse_query_2<T1: FromStr, T2: FromStr>(input: String) -> Result<(T1, T2), Status>
-where
-    <T1 as FromStr>::Err: std::fmt::Display,
-    <T2 as FromStr>::Err: std::fmt::Display,
-{
-    match input.split(',').collect::<Vec<&str>>().as_slice() {
+fn parse_query_2<T1: FromStr, T2: FromStr>(input: &str) -> Result<(T1, T2), Status>
+where
+    T1::Err: std::fmt::Display,
+    T2::Err: std::fmt::Display,
+{
+    let mut parts = input.splitn(2, ',');
+    if let (Some(v1), Some(v2)) = (parts.next(), parts.next()) {
         Ok((
             parse_query::<T1>(v1.to_string())?,
             parse_query::<T2>(v2.to_string())?,
         ))
-    },
-    _ => Err(Status::invalid_argument("invalid page_token format")),
-    }
+    } else {
+        Err(Status::invalid_argument("invalid page_token format"))
+    }
 }
multichain-aggregator/multichain-aggregator-proto/swagger/v1/multichain-aggregator.swagger.yaml (1)

252-280: Ensure all new schemas are properly documented

The newly added schemas v1ListAddressesResponse, v1MarketplaceDapp, and v1Pagination lack descriptions for their properties. Providing clear descriptions helps users understand the API and improves maintainability.

Consider adding descriptions to the properties:

v1ListAddressesResponse:
  type: object
  properties:
    addresses:
      type: array
      items:
        type: object
        $ref: '#/definitions/v1Address'
      description: List of address objects matching the search criteria.
    pagination:
      $ref: '#/definitions/v1Pagination'
      description: Pagination information for the response.
v1MarketplaceDapp:
  type: object
  properties:
    id:
      type: string
      description: Unique identifier of the DApp.
    title:
      type: string
      description: Title of the DApp.
    logo:
      type: string
      description: URL to the DApp's logo image.
    short_description:
      type: string
      description: Brief description of the DApp.
v1Pagination:
  type: object
  properties:
    page_token:
      type: string
      description: Token to retrieve the next page of results.
    page_size:
      type: integer
      format: int64
      description: Number of results returned per page.
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3a62e6a and e28c105.

⛔ Files ignored due to path filters (1)
  • multichain-aggregator/Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (17)
  • multichain-aggregator/Cargo.toml (1 hunks)
  • multichain-aggregator/justfile (1 hunks)
  • multichain-aggregator/multichain-aggregator-logic/Cargo.toml (1 hunks)
  • multichain-aggregator/multichain-aggregator-logic/src/dapp_client.rs (1 hunks)
  • multichain-aggregator/multichain-aggregator-logic/src/lib.rs (2 hunks)
  • multichain-aggregator/multichain-aggregator-logic/src/repository/addresses.rs (4 hunks)
  • multichain-aggregator/multichain-aggregator-logic/src/search.rs (2 hunks)
  • multichain-aggregator/multichain-aggregator-logic/src/types/dapp.rs (1 hunks)
  • multichain-aggregator/multichain-aggregator-logic/src/types/mod.rs (1 hunks)
  • multichain-aggregator/multichain-aggregator-logic/src/types/search_results.rs (3 hunks)
  • multichain-aggregator/multichain-aggregator-proto/proto/v1/api_config_http.yaml (1 hunks)
  • multichain-aggregator/multichain-aggregator-proto/proto/v1/multichain-aggregator.proto (3 hunks)
  • multichain-aggregator/multichain-aggregator-proto/swagger/v1/multichain-aggregator.swagger.yaml (3 hunks)
  • multichain-aggregator/multichain-aggregator-server/Cargo.toml (2 hunks)
  • multichain-aggregator/multichain-aggregator-server/src/server.rs (2 hunks)
  • multichain-aggregator/multichain-aggregator-server/src/services/multichain_aggregator.rs (2 hunks)
  • multichain-aggregator/multichain-aggregator-server/src/settings.rs (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
  • multichain-aggregator/multichain-aggregator-logic/src/types/mod.rs
  • multichain-aggregator/multichain-aggregator-server/Cargo.toml
  • multichain-aggregator/justfile
  • multichain-aggregator/multichain-aggregator-logic/Cargo.toml
  • multichain-aggregator/multichain-aggregator-logic/src/types/search_results.rs
  • multichain-aggregator/Cargo.toml
  • multichain-aggregator/multichain-aggregator-logic/src/lib.rs
  • multichain-aggregator/multichain-aggregator-server/src/settings.rs
  • multichain-aggregator/multichain-aggregator-server/src/server.rs
  • multichain-aggregator/multichain-aggregator-proto/proto/v1/api_config_http.yaml
  • multichain-aggregator/multichain-aggregator-logic/src/types/dapp.rs
  • multichain-aggregator/multichain-aggregator-proto/proto/v1/multichain-aggregator.proto
  • multichain-aggregator/multichain-aggregator-logic/src/dapp_client.rs
🔇 Additional comments (1)
multichain-aggregator/multichain-aggregator-logic/src/repository/addresses.rs (1)

83-84: 🛠️ Refactor suggestion

Ensure consistent ordering for pagination

The query orders results by Hash ascending, then ChainId ascending, but the pagination key is a tuple (AddressAlloy, ChainId), where AddressAlloy represents the address hash. This ordering should match the pagination key to ensure consistent and predictable pagination results. However, if the pagination key's order is (ChainId, AddressAlloy), then the ordering should be adjusted accordingly.

Consider adjusting the ordering to match the pagination key:

-            .order_by_asc(Column::Hash)
-            .order_by_asc(Column::ChainId)
+            .order_by_asc(Column::ChainId)
+            .order_by_asc(Column::Hash)

Copy link
Contributor

@bragov4ik bragov4ik left a comment

Choose a reason for hiding this comment

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

LGTM, very nice and understandable 👍

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

♻️ Duplicate comments (1)
multichain-aggregator/multichain-aggregator-server/src/settings.rs (1)

73-76: ⚠️ Potential issue

Replace hardcoded localhost URL with environment variable.

The default DappClient URL is still hardcoded to "http://localhost:8050". This could cause issues in different environments.

Consider using an environment variable with a fallback:

-                    url: Url::parse("http://localhost:8050").unwrap(),
+                    url: Url::parse(&std::env::var("DAPP_CLIENT_URL")
+                        .unwrap_or_else(|_| "http://localhost:8050".to_string()))
+                        .expect("Invalid DAPP_CLIENT_URL"),
🧹 Nitpick comments (6)
multichain-aggregator/multichain-aggregator-server/config/example.toml (1)

4-5: LGTM! Consider adding a comment about environment override.

The DApp client configuration section is properly structured. Consider adding a comment indicating that this URL can be overridden using the MULTICHAIN_AGGREGATOR__SERVICE__DAPP_CLIENT__URL environment variable.

multichain-aggregator/README.md (2)

35-35: Fix grammar in database creation description.

The current text "Create database if doesn't exist" has a missing subject.

-| `MULTICHAIN_AGGREGATOR__DATABASE__CREATE_DATABASE`       |                          | Create database if doesn't exist    | `false`       |
+| `MULTICHAIN_AGGREGATOR__DATABASE__CREATE_DATABASE`       |                          | Create database if it doesn't exist | `false`       |
🧰 Tools
🪛 LanguageTool

[grammar] ~35-~35: It seems that a pronoun is missing.
Context: ... | Create database if doesn't exist | false | | `M...

(IF_VB)


38-39: Add descriptions for pagination settings.

The description column is empty for the pagination-related environment variables. Consider adding explanations:

-| `MULTICHAIN_AGGREGATOR__SERVICE__API__DEFAULT_PAGE_SIZE` |                          |                                     | `50`          |
-| `MULTICHAIN_AGGREGATOR__SERVICE__API__MAX_PAGE_SIZE`     |                          |                                     | `100`         |
+| `MULTICHAIN_AGGREGATOR__SERVICE__API__DEFAULT_PAGE_SIZE` |                          | Default number of items per page    | `50`          |
+| `MULTICHAIN_AGGREGATOR__SERVICE__API__MAX_PAGE_SIZE`     |                          | Maximum allowed items per page      | `100`         |
multichain-aggregator/multichain-aggregator-server/src/services/multichain_aggregator.rs (3)

42-45: Add documentation for pagination limits

Consider adding documentation to explain the default page size and maximum limits from api_settings. This would help future maintainers understand the pagination constraints.

+    /// Normalizes the page size to be within acceptable bounds.
+    /// 
+    /// - Uses default_page_size from api_settings if size is None
+    /// - Ensures the size is between 1 and max_page_size from api_settings
     fn normalize_page_size(&self, size: Option<u32>) -> u32 {

102-104: Enhance page token format robustness

The current page token format using a simple comma separator could be fragile. Consider using a more robust format like base64-encoded JSON or a structured format that's less likely to have parsing issues.

-                page_token: format!("{},{}", a.to_checksum(None), c),
+                page_token: base64::encode(serde_json::to_string(&PageToken { address: a.to_checksum(None), chain_id: c }).unwrap()),

127-149: Add documentation for parsing utilities

These utility functions would benefit from documentation explaining their purpose and expected input formats.

+    /// Parses a string into a type that implements FromStr.
+    /// 
+    /// Returns a Status::invalid_argument error if parsing fails.
     #[inline]
     fn parse_query<T: FromStr>(input: String) -> Result<T, Status>

+    /// Parses a comma-separated string into a tuple of two values.
+    /// 
+    /// Expected format: "value1,value2"
+    /// Returns a Status::invalid_argument error if parsing fails or format is invalid.
     #[inline]
     fn parse_query_2<T1: FromStr, T2: FromStr>(input: String) -> Result<(T1, T2), Status>
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e28c105 and d3c2d29.

📒 Files selected for processing (5)
  • multichain-aggregator/README.md (1 hunks)
  • multichain-aggregator/multichain-aggregator-server/config/example.toml (1 hunks)
  • multichain-aggregator/multichain-aggregator-server/src/server.rs (2 hunks)
  • multichain-aggregator/multichain-aggregator-server/src/services/multichain_aggregator.rs (2 hunks)
  • multichain-aggregator/multichain-aggregator-server/src/settings.rs (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • multichain-aggregator/multichain-aggregator-server/src/server.rs
🧰 Additional context used
🪛 LanguageTool
multichain-aggregator/README.md

[grammar] ~35-~35: It seems that a pronoun is missing.
Context: ... | Create database if doesn't exist | false | | `M...

(IF_VB)

🔇 Additional comments (5)
multichain-aggregator/multichain-aggregator-server/src/settings.rs (1)

25-40: LGTM! Well-structured pagination settings.

The pagination settings implementation is clean and follows Rust best practices:

  • Good use of default trait implementations
  • Reasonable default values (50 for default, 100 for max)
  • Proper use of serde attributes for configuration

Also applies to: 86-92

multichain-aggregator/multichain-aggregator-server/src/services/multichain_aggregator.rs (4)

1-7: LGTM: Clean integration of new dependencies and struct fields

The new imports and struct fields are well-organized and properly typed for the enhanced search functionality.

Also applies to: 10-11, 22-23


27-39: LGTM: Constructor properly initialized with new dependencies

The constructor correctly handles the new dependencies while maintaining existing functionality.


78-107: Consider renaming for clarity

As mentioned in the previous review, the method name list_addresses might be misleading since it's performing a search operation. Consider renaming to search_addresses for better clarity.


115-121: LGTM: Clean integration of dapp_client

The quick_search implementation properly integrates the dapp_client while maintaining consistent error handling patterns.

@lok52 lok52 merged commit 57c1884 into main Dec 18, 2024
5 checks passed
@lok52 lok52 deleted the lok52/multichain-search-api branch December 18, 2024 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants