feat(multichain): update token-info client #1208
Conversation
WalkthroughThe pull request introduces significant changes to the multichain-aggregator project by integrating the 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 (6)
multichain-aggregator/multichain-aggregator-logic/src/clients/mod.rs (1)
2-7: Consider selective re-exports for maintainability.Re-exporting the entire contents of
contracts_info_protomay make it less obvious which items are actually used, potentially complicating future refactoring. Consider re-exporting specific structs or modules that are actively consumed to maintain a clearer API surface.multichain-aggregator/multichain-aggregator-server/src/settings.rs (2)
60-67: Ensure default timeout is documented.Including
#[serde(default = "default_token_info_timeout")]helps avoid missing configuration, but ensure the documentation clarifies the default timeout's behavior.
112-114: Potentially adjust default timeout based on environment.Five seconds may be insufficient or too generous in some deployments. Consider making it configurable at build time or environment-based.
multichain-aggregator/multichain-aggregator-logic/src/search.rs (1)
53-58: Validate the default pagination parameters.Specifying
page_size = Some(100)and settingchain_id = Noneexpands the search across all chains. Confirm this aligns with intended usage and that 100 is the appropriate default page size. If needed, consider hooking into a configurable setting or having a safe upper limit.multichain-aggregator/multichain-aggregator-server/src/services/multichain_aggregator.rs (1)
30-30: Validate client reuse.Changing
token_info_clientto a specializedtoken_info::Clientclarifies its purpose. Verify that reusing a single client instance does not introduce synchronization or performance issues if it’s used across multiple concurrent requests.multichain-aggregator/multichain-aggregator-server/Cargo.toml (1)
30-33: Maintain clarity of feature flags.Expanding feature lists for
blockscout-service-launcherimproves readability. Confirm that the newly listed features ("test-server", "test-database") are required for all environments or limit them to only development and testing where appropriate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
multichain-aggregator/Cargo.toml(1 hunks)multichain-aggregator/multichain-aggregator-logic/Cargo.toml(1 hunks)multichain-aggregator/multichain-aggregator-logic/src/clients/mod.rs(1 hunks)multichain-aggregator/multichain-aggregator-logic/src/clients/token_info.rs(0 hunks)multichain-aggregator/multichain-aggregator-logic/src/search.rs(3 hunks)multichain-aggregator/multichain-aggregator-logic/src/types/token_info.rs(2 hunks)multichain-aggregator/multichain-aggregator-server/Cargo.toml(1 hunks)multichain-aggregator/multichain-aggregator-server/src/server.rs(1 hunks)multichain-aggregator/multichain-aggregator-server/src/services/multichain_aggregator.rs(4 hunks)multichain-aggregator/multichain-aggregator-server/src/settings.rs(4 hunks)
💤 Files with no reviewable changes (1)
- multichain-aggregator/multichain-aggregator-logic/src/clients/token_info.rs
🧰 Additional context used
🪛 GitHub Actions: Test, lint and docker (multichain-aggregator)
multichain-aggregator/Cargo.toml
[error] Failed to load manifest for workspace member. Dependency 'contracts-info-proto' could not be found because its Cargo.toml file is missing at '/home/runner/work/blockscout-rs/blockscout-rs/contracts-info-proto/Cargo.toml'
🔇 Additional comments (13)
multichain-aggregator/multichain-aggregator-logic/src/types/token_info.rs (2)
13-16: Validate optional fields in unit tests.The
token_nameandtoken_symbolfields are enforced viaok_or_else(...). Ensure there are test cases covering missing values to verify the error handling logic.
26-29: Confirm chain ID parsing for edge cases.While the
try_into()approach is correct, consider adding coverage for edge values (e.g.,0, negative numbers if relevant, etc.) to ensure that this error handling is robust.multichain-aggregator/multichain-aggregator-server/src/server.rs (1)
72-76: Verify error handling for asynchronous client creation.
token_info::Client::new(config).awaitlikely returns aResult; confirm that any potential errors (e.g., invalid URL, timeout) are handled or bubbled up. If needed, add a?or handle it explicitly.multichain-aggregator/multichain-aggregator-server/src/settings.rs (2)
7-8: Good use of serde_as for custom serialization.This makes it straightforward to store duration fields in seconds while maintaining a
std::time::Durationin code.
93-93: Highlight usage of a default timeout in the default settings.The default function sets a fallback of 5 seconds. Confirm this is clear in any configuration docs to prevent confusion.
multichain-aggregator/multichain-aggregator-logic/src/search.rs (2)
4-4: Confirm compatibility with external references.Importing
ClientandSearchTokenInfosRequestfrom the new proto-based module is a solid step. Ensure all call sites and references (e.g., testing code, integration points) are updated to use these new imports to avoid potential mismatches.
41-41: Ensure uniform interface updates.Switching from
&HttpApiClientto&Clientrequires consistent updates throughout the codebase (e.g., function calls, trait implementations). Double-check that no references toHttpApiClientremain where they should now beClient.multichain-aggregator/multichain-aggregator-server/src/services/multichain_aggregator.rs (2)
13-13: Check the scoping of re-exports.Re-exporting
endpoints::SearchTokenInfosandproto::SearchTokenInfosRequestconsolidates usage undertoken_info. Confirm that any existing aliases or references to removed modules (if any) are now correctly mapped here.
39-39: Maintain consistency in constructor parameters.Passing
token_info_client: token_info::Clientconsistently across constructors helps reduce confusion. Double-check that all services and tests requiring this struct are aligned with the new signature.multichain-aggregator/multichain-aggregator-logic/Cargo.toml (1)
24-24: Confirm version alignment.Adding
contracts-info-proto = { workspace = true }indicates a new shared dependency. Verify that the workspace’scontracts-info-protoversion is compatible with the logic requiring it, and ensure no version drift exists in related crates.multichain-aggregator/multichain-aggregator-server/Cargo.toml (2)
22-22: Evaluate necessity ofserde_with.
serde_withcan simplify serialization and deserialization of certain patterns (e.g., flattening, specialized date/time formats). Confirm that it’s indeed needed and used. Otherwise, removing it can help keep dependencies minimal.
35-35: Confirm consistency inreqwestfeatures.Refactoring
reqwestusage withfeatures = ["json"]is clean. Verify no other feature sets (e.g.,gzip) are required for production usage to avoid unexpected runtime failures.multichain-aggregator/Cargo.toml (1)
34-34: Verify the http-client feature configuration.Let's verify that the
http-clientfeature is correctly defined in thecontracts-info-protocrate.🧰 Tools
🪛 GitHub Actions: Test, lint and docker (multichain-aggregator)
[error] Failed to load manifest for workspace member. Dependency 'contracts-info-proto' could not be found because its Cargo.toml file is missing at '/home/runner/work/blockscout-rs/blockscout-rs/contracts-info-proto/Cargo.toml'
| let token_info_search_endpoint = SearchTokenInfos::new(SearchTokenInfosRequest { | ||
| query: inner.q.to_string(), | ||
| chain_id, | ||
| page_size: inner.page_size, | ||
| page_token: inner.page_token, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement thorough error handling & boundary checks.
Using SearchTokenInfos::new(...) with user-supplied parameters is good. Ensure validations are performed on inner.q, inner.page_size, etc., to avoid potential large or invalid requests. Consider falling back to default values or returning errors if the user input is out of range.
| # clients | ||
| contracts-info-proto = { path = "../contracts-info-proto", features = ["http-client"] } |
There was a problem hiding this comment.
Fix the dependency path to resolve build failure.
The pipeline is failing because the contracts-info-proto dependency cannot be found at the specified path ../contracts-info-proto. This indicates that either:
- The repository structure is different from what's expected
- The dependency path needs to be adjusted
To resolve this, we need to verify the correct path to the dependency. Please run this script to check the repository structure:
#!/bin/bash
# Description: Check repository structure and find the contracts-info-proto directory
echo "Searching for contracts-info-proto directory..."
fd -t d "contracts-info-proto$"
echo -e "\nChecking if the directory exists at ../contracts-info-proto..."
if [ -d "../contracts-info-proto" ]; then
echo "Directory exists at ../contracts-info-proto"
if [ -f "../contracts-info-proto/Cargo.toml" ]; then
echo "Cargo.toml exists in the directory"
else
echo "Cargo.toml is missing"
fi
else
echo "Directory does not exist at ../contracts-info-proto"
fi🧰 Tools
🪛 GitHub Actions: Test, lint and docker (multichain-aggregator)
[error] Failed to load manifest for workspace member. Dependency 'contracts-info-proto' could not be found because its Cargo.toml file is missing at '/home/runner/work/blockscout-rs/blockscout-rs/contracts-info-proto/Cargo.toml'
lok52
changed the title
Summary by CodeRabbit
Release Notes
Dependencies
contracts-info-protodependency across multiple packagesClient Modifications
Code Refactoring