Conversation
WalkthroughEncapsulates and validates fetch configuration: Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as caller (tests / init)
participant FetchConfig as FetchConfig::new
participant Valid as Validation
participant TokenFetch as fetch_erc20_metadata_concurrent
participant LocalDbErr as LocalDbError mapping
Caller->>FetchConfig: call new(chunk_size, max_req, max_blocks, max_retries)
FetchConfig->>Valid: validate params
alt valid
Valid-->>FetchConfig: Ok(FetchConfig)
FetchConfig-->>Caller: returns FetchConfig
Caller->>TokenFetch: pass &FetchConfig
TokenFetch->>FetchConfig: call accessors (chunk_size(), max_concurrent_requests(), max_retry_attempts())
TokenFetch-->>Caller: proceed with fetch using values
else invalid
Valid-->>FetchConfig: Err(FetchConfigError)
FetchConfig-->>Caller: returns FetchConfigError
Caller->>LocalDbErr: propagate via From
LocalDbErr-->>Caller: LocalDbError::FetchConfigError
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (3)crates/*/{src,tests,benches,examples}/**/*.rs📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
crates/**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (1)📚 Learning: 2025-07-09T07:42:48.458ZApplied to files:
🧬 Code graph analysis (1)crates/common/src/local_db/mod.rs (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🔇 Additional comments (6)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
crates/common/src/local_db/token_fetch.rs (1)
24-61: Simplify stream: usetry_collectto short‑circuit and avoid a second pass.Removes the intermediate
Vec<Result<..>>and the manual loop.Apply:
- let results: Vec<Result<(Address, TokenInfo), LocalDbError>> = - futures::stream::iter(missing_addrs.into_iter().map(|addr| { + let successes: Vec<(Address, TokenInfo)> = + futures::stream::iter(missing_addrs.into_iter().map(|addr| { let rpcs = rpcs.clone(); async move { let erc20 = ERC20::new(rpcs, addr); let result = retry_with_backoff( || { let erc20 = erc20.clone(); async move { erc20.token_info(None).await } }, max_attempts, should_retry_token_error, ) .await; match result { - Ok(info) => Ok((addr, info)), + Ok(info) => Ok::<_, LocalDbError>((addr, info)), Err(RetryError::Operation(err)) => Err(LocalDbError::CustomError(format!( "Failed to fetch token info for 0x{:x} after {} attempts: {}", addr, max_attempts, err ))), Err(RetryError::Config { message }) => Err(LocalDbError::Config { message }), } } })) - .buffer_unordered(concurrency) - .collect() - .await; - - let mut successes: Vec<(Address, TokenInfo)> = Vec::new(); - for r in results { - match r { - Ok(pair) => successes.push(pair), - Err(e) => return Err(e), - } - } - Ok(successes) + .buffer_unordered(concurrency) + .try_collect() + .await?; + Ok(successes)And add the missing import:
-use futures::StreamExt; +use futures::{StreamExt, TryStreamExt};crates/common/src/local_db/fetch.rs (3)
323-357: Retry predicate: include transient HTTP/timeout conditions.Increase resilience by retrying on common transient failures.
Apply:
-fn should_retry_local_db_error(error: &LocalDbError) -> bool { - matches!(error, LocalDbError::Rpc(_)) -} +fn should_retry_local_db_error(error: &LocalDbError) -> bool { + matches!( + error, + LocalDbError::Rpc(_) + | LocalDbError::Timeout + | LocalDbError::HttpStatus { status } + if *status == 429 || (500..=599).contains(status) + ) +}
359-367: Minor perf nit: avoid repeated parses in sort.Precompute block/log indices once to reduce duplicate parsing in comparator.
398-423: Convert block number overflow toInvalidBlockNumbererror instead of panicking.The conversion
block_u256.to::<u64>()is a checked conversion that panics on overflow when the block number exceedsu64::MAX. Instead, use a fallible conversion method or add overflow detection to returnInvalidBlockNumbererror consistently with other parsing failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
crates/common/src/local_db/fetch.rs(13 hunks)crates/common/src/local_db/mod.rs(1 hunks)crates/common/src/local_db/token_fetch.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: git-clean
- GitHub Check: test
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: Deploy-Preview-Push
- GitHub Check: Deploy-Docs-Preview
🔇 Additional comments (6)
crates/common/src/local_db/mod.rs (1)
13-13: Additive re‑export looks good.Publicly exposing
FetchConfigErrormatches the new constructor/validation flow and keeps error routing coherent viaFrom<FetchConfigError> for LocalDbError.crates/common/src/local_db/token_fetch.rs (1)
21-22: LGTM on accessor usage.Switch to getters is correct and enforces validated, non‑zero values from the constructor/default.
crates/common/src/local_db/fetch.rs (4)
12-25: Good dedicated config‑validation errors.Clear, precise variants with user‑friendly messages. Suits constructor validation.
27-33: Error routing is coherent.
From<FetchConfigError> for LocalDbErrorkeeps callers on a single error type.
49-78: Constructor validation is correct.All knobs validated for non‑zero; prevents runtime panics (e.g., zero concurrency) downstream.
288-311: Chunking logic handles boundaries well; constructor pattern properly enforced throughout codebase.Use of
saturating_*andminavoids overflow; loop termination coversu64::MAXedge. Verification confirms allFetchConfiginstantiations across the codebase use either thenew()constructor orDefaulttrait—no direct struct literals bypassing validation were found.
| impl From<FetchConfigError> for LocalDbError { | ||
| fn from(error: FetchConfigError) -> Self { | ||
| LocalDbError::Config { | ||
| message: error.to_string(), | ||
| } | ||
| } |
There was a problem hiding this comment.
Why still squashing this to a string?
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
|
Tip For best results, initiate chat on the files or code changes. SIZE=M 🧠 Learnings used |
3 participants
Motivation
See issue: #2221
Solution
Constructor is not used at the moment but we are going to switch to it when we implement the local db configuration parsing with #2247
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit