fix: Improve pools and rewards endpoints - #784
Conversation
WalkthroughThis PR modifies account reward epoch handling and introduces offchain metadata fetching for pools. The accounts route offsets epochs by -1 during reward mapping with a TODO comment. The pools route adds a new PoolOffchainMetadata struct and async metadata fetching function, then refactors all_extended to compute extended pool data by aggregating AccountState live stakes and fetching metadata from URLs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant all_extended as all_extended<br/>(pools route)
participant Archive as Archive<br/>(TemporalKey)
participant HTTP as HTTP Client<br/>(metadata URL)
participant Response as JSON Response
Client->>all_extended: Request extended pool data
all_extended->>all_extended: Aggregate live stakes from<br/>AccountState entries
all_extended->>all_extended: Calculate circulating_supply<br/>& optimal pparams
loop For each PoolState entry
all_extended->>Archive: Fetch StakeLog via TemporalKey
Archive-->>all_extended: active_stake data
all_extended->>HTTP: GET pool metadata from URL
HTTP-->>all_extended: PoolOffchainMetadata (or timeout)
all_extended->>all_extended: Compute pool metrics<br/>(live_stake, saturation, etc)
all_extended->>all_extended: Enrich with metadata
end
all_extended->>Response: Return ordered paginated results
Response-->>Client: PoolListExtendedInner[] with metadata
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 (2)
crates/minibf/src/routes/pools.rs (2)
43-57: Consider error logging and caching for metadata fetching.The
pool_offchain_metadatafunction silently returnsNoneon any error (network failures, timeouts, deserialization errors), which could make debugging difficult when metadata doesn't load.Additionally, repeated calls for the same pool will re-fetch the metadata each time, which is inefficient.
Consider these improvements:
- Add logging for fetch failures to aid troubleshooting
- Implement a caching layer (in-memory or persistent) to avoid repeated HTTP requests
- Consider using concurrent requests when fetching metadata for multiple pools
Apply this diff to add basic error logging:
async fn pool_offchain_metadata(url: &str) -> Option<PoolOffchainMetadata> { let client = reqwest::Client::builder() .timeout(Duration::from_secs(10)) .user_agent("Dolos MiniBF") .build() - .ok()?; + .inspect_err(|e| tracing::warn!("Failed to build HTTP client: {}", e)) + .ok()?; - let res = client.get(url).send().await.ok()?; + let res = client.get(url).send().await + .inspect_err(|e| tracing::warn!("Failed to fetch metadata from {}: {}", url, e)) + .ok()?; if res.status() != StatusCode::OK { + tracing::warn!("Non-OK status {} from metadata URL: {}", res.status(), url); return None; } - res.json().await.ok() + res.json().await + .inspect_err(|e| tracing::warn!("Failed to parse metadata JSON from {}: {}", url, e)) + .ok() }
59-65: Consider making metadata fields optional.All fields in
PoolOffchainMetadataare required (Stringrather thanOption<String>). If the fetched JSON is missing any field, deserialization will fail andpool_offchain_metadatawill returnNone, hiding the partially-available metadata.Consider making fields optional to gracefully handle incomplete metadata:
#[derive(Serialize, Deserialize, Clone)] pub struct PoolOffchainMetadata { - pub name: String, - pub description: String, - pub ticker: String, - pub homepage: String, + pub name: Option<String>, + pub description: Option<String>, + pub ticker: Option<String>, + pub homepage: Option<String>, }This would allow partial metadata to be displayed when pools provide incomplete information, improving user experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/minibf/src/routes/accounts.rs(1 hunks)crates/minibf/src/routes/pools.rs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
crates/minibf/src/routes/accounts.rs (1)
crates/cardano/src/model.rs (1)
epoch(193-198)
crates/minibf/src/routes/pools.rs (4)
crates/cardano/src/model.rs (11)
new(148-157)new(542-552)new(1570-1580)key(757-759)key(1890-1924)live(201-203)epoch(193-198)from(1613-1615)from(1619-1621)from(1625-1627)from(1631-1634)crates/core/src/state.rs (8)
new(212-218)key(139-139)from(16-21)from(25-27)from(31-33)from(37-39)from(64-69)from(83-85)crates/minibf/src/mapping.rs (5)
new(481-493)new(1451-1466)new(1553-1611)new(1661-1671)bech32_pool(104-106)crates/cardano/src/lib.rs (3)
state(422-423)load_epoch(415-415)load_epoch(421-427)
🔇 Additional comments (3)
crates/minibf/src/routes/pools.rs (2)
175-177: Verify division by zero cannot occur in saturation calculation.Line 176 divides by
circulating_supplywithout checking if it's zero. Ifcirculating_supplyis zero, this will produceInfinityorNaN.Please verify that
circulating_supplyfrominitial_pots.circulating()(line 95) can never be zero, or add a guard:live_saturation: live - .map(|x| x as f64 * optimal as f64 / circulating_supply as f64) + .map(|x| { + if circulating_supply == 0 { + 0.0 + } else { + x as f64 * optimal as f64 / circulating_supply as f64 + } + }) .unwrap_or_default(),
155-166: Verify active stake epoch offset is correct.Line 160 fetches the active stake from
epoch - 1, similar to the epoch offset inaccounts.rs. While this may be intentional (active stake typically lags behind the current epoch in Cardano's reward cycle), it should be verified for correctness.Please confirm:
- Is the active stake intentionally fetched from the previous epoch?
- Does this align with Cardano's stake snapshot and delegation cycle?
- Should this be documented with a comment explaining the offset?
Additionally, consider adding error context when the archive read fails:
let Ok(active) = domain.archive().read_log_typed::<StakeLog>( StakeLog::NS, &(TemporalKey::from(active_slot), key.clone()).into(), ) else { + tracing::error!("Failed to read active stake log for pool at epoch {}", epoch); return Err(StatusCode::INTERNAL_SERVER_ERROR.into()); };crates/minibf/src/routes/accounts.rs (1)
569-570: I'll search for more context on the epoch numbering and the iterator implementation to understand why the offset is necessary.Address epoch offset timing per the TODO comment.
The epoch offset (
epoch as i32 - 1) at line 570 is applied during read time as documented by the TODO comment. This is a design choice that warrants clarification:
- The offset maps internal epoch values to Blockfrost API format (e.g., print(account_rewards[0].epoch) returns 221), indicating the offset aligns with external API expectations.
- Only one read path applies this transformation (no inconsistency risk identified across multiple reward access patterns).
- The TODO indicates this should be handled during write operations, which is a valid refactoring consideration but not an immediate correctness issue.
Clarify: Is the offset necessary due to a mismatch between how rewards are stored internally (one-indexed) versus how Blockfrost API expects them (zero-indexed)? If so, document this mapping or plan the write-time migration as part of your refactoring roadmap.
| for x in domain | ||
| .iter_cardano_entities::<AccountState>(None) | ||
| .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)? | ||
| { | ||
| let (_, state) = x.map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?; | ||
| if let Some(PoolDelegation::Pool(hash)) = state.pool.live() { | ||
| let stake = state.stake.live().map(|x| x.total()).unwrap_or(0); | ||
| live_stake_map | ||
| .entry(*hash) | ||
| .and_modify(|entry| *entry += stake) | ||
| .or_insert(stake); | ||
| }; | ||
| } |
There was a problem hiding this comment.
Critical performance concern: iterating all accounts on every request.
The function iterates over all AccountState entities to build the live_stake_map on every call to all_extended. With a large number of accounts, this will:
- Consume significant memory by loading all account states
- Take substantial time to process, blocking the request
- Scale poorly as the number of accounts grows
This is particularly problematic since this endpoint will be called frequently with different pagination parameters.
Consider these alternatives:
- Maintain a materialized aggregate: Pre-compute and store pool live stakes during write operations, updating incrementally as delegations change
- Use a database query: If using a database, leverage SQL aggregation to compute stakes efficiently
- Cache the live stake map: Cache the computed map with a TTL, refreshing periodically rather than on every request
Would you like me to help design a caching or materialization strategy?
🤖 Prompt for AI Agents
crates/minibf/src/routes/pools.rs around lines 79-91: the code iterates all
AccountState entities on every request to build live_stake_map which is O(N) and
will not scale; replace this hot-path work with one of the following fixes: (1)
read precomputed per-pool live stake aggregates maintained/updated on write
operations (recommended) by updating the aggregate whenever an account's
delegation or stake changes, (2) perform a single database-side aggregation
query that groups by pool and sums stakes instead of loading each AccountState
into memory, or (3) add an in-memory cached live_stake_map with a TTL or
background refresh task and serve requests from the cache; implement one of
these approaches and remove the per-request full iteration so the endpoint
responds from the aggregate/cache/query rather than scanning all accounts.
| for (key, pool) in pools { | ||
| let poolhex = hex::encode(pool.operator); | ||
| let pool_id = bech32_pool(pool.operator)?; | ||
| let params = pool.snapshot.live().map(|x| x.params.clone()); | ||
| let metadata = match params.as_ref() { | ||
| Some(x) => match x.pool_metadata.as_ref() { | ||
| Some(y) => { | ||
| let out = match pool_offchain_metadata(&y.url).await { | ||
| Some(meta) => json!({ | ||
| "url": y.url, | ||
| "hash": y.hash, | ||
| "ticker": meta.ticker, | ||
| "name": meta.name, | ||
| "description": meta.description, | ||
| "homepage": meta.homepage | ||
| }), | ||
| None => json!({ | ||
| "url": y.url, | ||
| "hash": y.hash, | ||
| "ticker": None::<String>, | ||
| "name": None::<String>, | ||
| "description": None::<String>, | ||
| "homepage": None::<String> | ||
| }), | ||
| }; | ||
|
|
||
| Some(Box::new(out)) | ||
| } | ||
| None => None, | ||
| }, | ||
| None => None, | ||
| }; | ||
|
|
||
| Ok(Json(mapped)) | ||
| // Fetch live and active stake logs | ||
| let live = live_stake_map.get(&pool.operator).copied(); | ||
| let Some(epoch) = pool.snapshot.epoch() else { | ||
| return Err(StatusCode::INTERNAL_SERVER_ERROR.into()); | ||
| }; | ||
| let active_slot = chain_summary.epoch_start(epoch - 1); | ||
| let Ok(active) = domain.archive().read_log_typed::<StakeLog>( | ||
| StakeLog::NS, | ||
| &(TemporalKey::from(active_slot), key.clone()).into(), | ||
| ) else { | ||
| return Err(StatusCode::INTERNAL_SERVER_ERROR.into()); | ||
| }; | ||
|
|
||
| out.push(PoolListExtendedInner { | ||
| pool_id, | ||
| hex: poolhex, | ||
| live_stake: live.map(|x| x.to_string()).unwrap_or("0".to_string()), | ||
| active_stake: active | ||
| .map(|x| x.total_stake.to_string()) | ||
| .unwrap_or("0".to_string()), | ||
| live_saturation: live | ||
| .map(|x| x as f64 * optimal as f64 / circulating_supply as f64) | ||
| .unwrap_or_default(), | ||
| blocks_minted: pool.blocks_minted_total as i32, | ||
| declared_pledge: params | ||
| .as_ref() | ||
| .map(|x| x.pledge.to_string()) | ||
| .unwrap_or_default(), | ||
| margin_cost: params | ||
| .as_ref() | ||
| .map(|x| rational_to_f64::<6>(&x.margin)) | ||
| .unwrap_or_default(), | ||
| fixed_cost: params | ||
| .as_ref() | ||
| .map(|x| x.cost.to_string()) | ||
| .unwrap_or_default(), | ||
| metadata, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Sequential HTTP requests will cause severe performance degradation.
Inside the main loop, pool_offchain_metadata (line 129) makes an HTTP request for each pool sequentially. If the pagination returns 100 pools, this performs 100 sequential HTTP requests, each with a 10-second timeout. This could take minutes to complete and severely degrade user experience.
Apply this refactor to fetch metadata concurrently:
+use futures::future::join_all;
+
pub async fn all_extended<D: Domain>(
Query(params): Query<PaginationParameters>,
State(domain): State<Facade<D>>,
) -> Result<Json<Vec<PoolListExtendedInner>>, Error>
where
Option<PoolState>: From<D::Entity>,
Option<AccountState>: From<D::Entity>,
{
// ... existing code to build pools vec ...
+ // Fetch all metadata concurrently
+ let metadata_futures: Vec<_> = pools
+ .iter()
+ .map(|(_, pool)| {
+ let url = pool
+ .snapshot
+ .live()
+ .and_then(|x| x.params.as_ref())
+ .and_then(|x| x.pool_metadata.as_ref())
+ .map(|x| x.url.clone());
+ async move {
+ match url {
+ Some(u) => pool_offchain_metadata(&u).await,
+ None => None,
+ }
+ }
+ })
+ .collect();
+
+ let metadata_results = join_all(metadata_futures).await;
let mut out = vec![];
- for (key, pool) in pools {
+ for ((key, pool), fetched_metadata) in pools.into_iter().zip(metadata_results) {
let poolhex = hex::encode(pool.operator);
let pool_id = bech32_pool(pool.operator)?;
let params = pool.snapshot.live().map(|x| x.params.clone());
let metadata = match params.as_ref() {
Some(x) => match x.pool_metadata.as_ref() {
Some(y) => {
- let out = match pool_offchain_metadata(&y.url).await {
+ let out = match fetched_metadata {
Some(meta) => json!({
"url": y.url,
"hash": y.hash,
"ticker": meta.ticker,
// ... rest of fieldsThis fetches all metadata concurrently, dramatically reducing response time.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/minibf/src/routes/pools.rs around lines 122 to 193, the code calls
pool_offchain_metadata sequentially inside the loop causing N HTTP requests to
run one-by-one; instead collect all pool metadata URLs up front, spawn
concurrent tasks (e.g., FuturesUnordered or join_all) to fetch
pool_offchain_metadata for each URL with proper timeout/error handling, await
all results, build a map from operator (or URL) to fetched metadata, and then
iterate the pools to construct PoolListExtendedInner using the pre-fetched
metadata rather than calling the HTTP function inline.
2 participants
Summary by CodeRabbit
Release Notes
New Features
Improvements