feat(api-client-framework): add blockscout services related helper definitions #1204
Conversation
WalkthroughThe pull request introduces a new optional feature for the API client framework, specifically targeting Blockscout services. A new Possibly related PRs
Poem
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
libs/api-client-framework/src/blockscout/client.rs (3)
29-29: Correct the error message to refer to the correct clientThe error message mentions "contracts-info client", which seems incorrect in this context. It should refer to "Blockscout client" to accurately reflect the service the client is connecting to.
Apply this diff to update the error message:
- panic!("Cannot establish a connection with contracts-info client: {err}") + panic!("Cannot establish a connection with Blockscout client: {err}")
62-63: Rename_statusfield tostatusThe field
_statusinHealthCheckResponseis used and therefore should not have a leading underscore, which is typically reserved for unused fields or variables. Renaming it tostatusenhances code clarity and eliminates the need for the#[serde(rename = "status")]attribute.Apply this diff to rename the field:
- #[serde(rename = "status")] - pub _status: i32, + pub status: i32,Ensure any references to
_statusare updated accordingly.
48-49: Correct indentation in documentation commentThe indentation in the documentation comment is inconsistent, which may affect readability and documentation generation tools.
Apply this diff to correct the indentation:
/// As we don't have protobuf generated structures here (they are only available inside a service), - /// we have to imitate the service health endpoint. +/// we have to imitate the service health endpoint.libs/api-client-framework/src/blockscout/config.rs (3)
22-31: Ensure proper error handling indeserialize_api_keyThe
deserialize_api_keyfunction correctly handles the conversion of the API key from aStringto aHeaderValue. However, consider logging or providing more context in the error message to aid troubleshooting if deserialization fails.Optionally, you can enhance the error handling as follows:
- string - .map(|value| HeaderValue::from_str(&value)) - .transpose() - .map_err(<D::Error as serde::de::Error>::custom) + match string { + Some(value) => HeaderValue::from_str(&value) + .map(Some) + .map_err(|e| D::Error::custom(format!("Invalid API key: {}", e))), + None => Ok(None), + }
33-61: SimplifyDebugimplementation forConfigManually implementing
Debugto skip themiddlewaresfield is acceptable. Alternatively, theDebugimplementation could be automatically derived using the#[derive(Debug)]attribute and annotating themiddlewaresfield with#[debug(skip)]from thederivativecrate.[refactor_suggestion_good_to_HAVE]
If it's acceptable to introduce an additional dependency, consider using the
derivativecrate for a cleaner implementation.use derivative::Derivative; #[derive(Clone, Deserialize, Derivative)] #[derivative(Debug)] pub struct Config { pub url: url::Url, #[serde(default, deserialize_with = "deserialize_api_key")] pub api_key: Option<HeaderValue>, #[serde(default = "defaults::http_timeout")] pub http_timeout: Duration, #[serde(default)] pub probe_url: bool, #[serde(skip_deserializing)] #[derivative(Debug = "ignore")] pub middlewares: Vec<Arc<dyn Middleware>>, }
99-102: Consider makinghttp_timeoutconfigurable at runtimeAllowing the
http_timeoutto be configurable can enhance flexibility, especially under varying network conditions. Ensure that this setting can be adjusted without requiring a recompilation of the application.libs/api-client-framework/src/lib.rs (1)
6-7: Enhance module documentation.Consider adding more context about what Blockscout is and when this module should be used.
/// Blockscout services related structs. /// Contains config and client definitions to be used by blockscout-rs services. +/// +/// Blockscout is a blockchain explorer that provides APIs for accessing blockchain data. +/// This module provides client implementations for interacting with Blockscout services.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
libs/api-client-framework/Cargo.toml(1 hunks)libs/api-client-framework/src/blockscout/client.rs(1 hunks)libs/api-client-framework/src/blockscout/config.rs(1 hunks)libs/api-client-framework/src/blockscout/mod.rs(1 hunks)libs/api-client-framework/src/lib.rs(1 hunks)
🔇 Additional comments (6)
libs/api-client-framework/src/blockscout/client.rs (1)
27-28: Verify the defaultHealthCheckRequestinitializationWhen performing the health check,
HealthCheckRequestis initialized withDefault::default(). Ensure that the service can handle an emptyserviceparameter or that the default initialization meets the API requirements.Consider verifying that the health check endpoint functions correctly with the default request.
libs/api-client-framework/src/blockscout/mod.rs (1)
1-5: Module structure looks goodThe addition of the
clientandconfigmodules and their public exports is well-structured, enhancing the modularity and accessibility of the Blockscout client functionality.libs/api-client-framework/src/blockscout/config.rs (1)
74-78: Ensure retry policy aligns with service requirementsWhen configuring the retry middleware with an exponential backoff strategy, verify that the maximum number of retries and the backoff parameters are suitable for the Blockscout service's rate limits and expected response times.
Consider reviewing the retry policy settings to ensure they adhere to best practices and do not inadvertently cause excessive load or violate service usage policies.
libs/api-client-framework/Cargo.toml (2)
21-25: LGTM! Well-structured feature definition.The blockscout feature is well-defined with appropriate dependencies for an API client implementation.
18-18: Verify reqwest-retry version compatibility.Please verify compatibility with reqwest v0.12.x as reqwest-retry v0.7.0 might have been tested with an older reqwest version.
Run this script to check compatibility:
libs/api-client-framework/src/lib.rs (1)
8-9: LGTM! Well-structured module definition.The module is correctly feature-gated and exposed as public, which is appropriate for a framework.
| #[serde(default = "defaults::http_timeout")] | ||
| pub http_timeout: Duration, |
There was a problem hiding this comment.
- How timeout is deserialized in that case (as seconds)?
- Can we make the default value configurable?
- Should we add
deny_unknown_fieldshere?
rimrakhimov
deleted the
rimrakhimov/api-framework/add-blockscout-definitions
branch
Summary by CodeRabbit
New Features
Improvements