feat: retry vote poll end times query and insert proof logs #81
Conversation
WalkthroughThis pull request introduces several modifications across multiple files, primarily focusing on implementing retry mechanisms for error handling in various querying methods within the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
🔇 Additional comments (6)src/backend_task/contested_names/query_dpns_contested_resources.rs (3)
The
The error handling is comprehensive with:
Also applies to: 75-76, 95-95, 68-68, 79-79
The error logging has been enhanced with more descriptive messages and proper formatting for both DPNS end times and vote contenders queries. Also applies to: 211-214 src/database/initialization.rs (2)
The version bump indicates schema changes for proof logs. Please ensure that:
The implementation includes proper table structure, indexing, and the new proof log table initialization. src/ui/dpns_contested_names_screen.rs (1)
The change improves clarity by showing "now" instead of "x seconds ago" for very recent updates. Also applies to: 536-540 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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (12)
src/backend_task/contested_names/query_dpns_contested_resources.rs (2)
12-12: Remove unused import.The
std::time::Instantimport appears to be unused in this file.-use std::time::Instant;
42-44: Consider adding debug logs for retry attempts.The retry mechanism is well-implemented, but adding debug logs for retry attempts would help with monitoring and debugging.
let mut retries = 0; let contested_resources = match ContestedResource::fetch_many(&sdk, query.clone()).await { Ok(contested_resources) => contested_resources, Err(e) => { tracing::error!("Error fetching contested resources: {}", e); if let dash_sdk::Error::Proof( dash_sdk::ProofVerifierError::GroveDBProofVerificationError { proof_bytes, path_query, height, time_ms, error, }, ) = &e { // ... existing code ... } if e.to_string().contains("try another server") || e.to_string().contains( "contract not found when querying from value with contract info", ) { retries += 1; + tracing::debug!("Retrying query attempt {}/{}", retries, MAX_RETRIES); if retries > MAX_RETRIES { tracing::error!("Max retries reached for query: {}", e); return Err(format!( "Error fetching contested resources after retries: {}", e )); } else { // Retry continue; } } else { return Err(format!("Error fetching contested resources: {}", e)); } } };Also applies to: 98-117
src/database/initialization.rs (3)
Line range hint
17-27: Replace println with proper loggingThe error handling logic is robust, but using
println!for logging isn't ideal in production code. Consider using a proper logging framework likelogcrate with appropriate log levels.- println!("Migration failed: {:?}", e); + log::error!("Migration failed: {:?}", e); - println!("Database reinitialized with default settings."); + log::info!("Database reinitialized with default settings.");
Line range hint
78-94: Handle potential panic in version checkingThe
unwrap_or(0)usage could be more explicit about the error case. Consider using proper error handling:- let version: u16 = conn - .query_row( - "SELECT database_version FROM settings WHERE id = 1", - [], - |row| row.get(0), - ) - .unwrap_or(0); // Default to version 0 if there's no version set + let version: u16 = conn + .query_row( + "SELECT database_version FROM settings WHERE id = 1", + [], + |row| row.get(0), + ) + .map_err(|e| { + log::warn!("Failed to get database version: {:?}", e); + rusqlite::Error::QueryReturnedNoRows + }) + .unwrap_or(0);
Line range hint
96-120: Consider implementing backup retention policyWhile the backup functionality is solid, consider:
- Implementing a retention policy to prevent unlimited backup accumulation
- Adding a cleanup routine for old backups
- Logging the total number of existing backups
Would you like me to help create an issue for implementing a backup retention strategy?
src/ui/dpns_contested_names_screen.rs (1)
415-419: Consider extracting the timestamp formatting logic into a helper method.The timestamp formatting logic is duplicated between
render_table_active_contestsandrender_table_past_contests. Consider extracting it into a helper method to improve maintainability and reduce duplication.impl DPNSContestedNamesScreen { + fn format_relative_time(&self, relative_time: String) -> String { + if relative_time.contains("seconds") { + "now".to_string() + } else { + relative_time + } + } }Then use it in both places:
- if relative_time.contains("seconds") { - ui.label("now"); - } else { - ui.label(relative_time); - } + ui.label(self.format_relative_time(relative_time));Also applies to: 536-540
src/backend_task/contested_names/query_dpns_vote_contenders.rs (3)
74-85: Simplify error handling using the?operatorYou can make the code more concise by using the
?operator to propagate errors, improving readability.Apply this diff to refactor the encoding of
encoded_query:-let encoded_query = match bincode::encode_to_vec( - &contenders_query, - bincode::config::standard(), -) -.map_err(|encode_err| { - tracing::error!("Error encoding query: {}", encode_err); - format!("Error encoding query: {}", encode_err) -}) { - Ok(encoded_query) => encoded_query, - Err(e) => return Err(e), -}; +let encoded_query = bincode::encode_to_vec( + &contenders_query, + bincode::config::standard(), +) +.map_err(|encode_err| { + tracing::error!("Error encoding query: {}", encode_err); + format!("Error encoding query: {}", encode_err) +})?;Similarly, refactor the encoding of
verification_path_query_bytes:-let verification_path_query_bytes = match bincode::encode_to_vec( - &path_query, - bincode::config::standard(), -) -.map_err(|encode_err| { - tracing::error!("Error encoding path_query: {}", encode_err); - format!("Error encoding path_query: {}", encode_err) -}) { - Ok(encoded_path_query) => encoded_path_query, - Err(e) => return Err(e), -}; +let verification_path_query_bytes = bincode::encode_to_vec( + &path_query, + bincode::config::standard(), +) +.map_err(|encode_err| { + tracing::error!("Error encoding path_query: {}", encode_err); + format!("Error encoding path_query: {}", encode_err) +})?;
98-112: Simplify error propagation when inserting proof log itemsUsing the
?operator can streamline error handling when inserting into the database.Apply this diff to simplify the code:
-if let Err(e) = self - .db - .insert_proof_log_item(ProofLogItem { - request_type: RequestType::GetContestedResourceIdentityVotes, - request_bytes: encoded_query, - verification_path_query_bytes, - height: *height, - time_ms: *time_ms, - proof_bytes: proof_bytes.clone(), - error: Some(error.clone()), - }) - .map_err(|e| e.to_string()) -{ - return Err(e); -} +self + .db + .insert_proof_log_item(ProofLogItem { + request_type: RequestType::GetContestedResourceIdentityVotes, + request_bytes: encoded_query, + verification_path_query_bytes, + height: *height, + time_ms: *time_ms, + proof_bytes: proof_bytes.clone(), + error: Some(error.clone()), + }) + .map_err(|e| e.to_string())?;
120-133: Remove unnecessaryelseblock afterreturnThe
elseblock following areturnis redundant and can be omitted to simplify the code.Apply this diff to remove the unnecessary
else:if retries > MAX_RETRIES { tracing::error!( "Max retries reached for query_dpns_vote_contenders: {}", e ); return Err(format!( "Error fetching vote contenders after retries: {}", e )); -} else { - continue; -} +} +// Continue to the next iteration +continue;src/backend_task/contested_names/query_ending_times.rs (1)
86-108: Reduce code duplication by refactoring encoding logic into a helper functionThe encoding of
end_time_queryandpath_queryshares similar logic, leading to duplicated code.Consider extracting the encoding and error handling into a helper function to improve readability and maintainability. For example:
fn encode_to_vec<T: Encode>(value: &T) -> Result<Vec<u8>, String> { bincode::encode_to_vec(value, bincode::config::standard()).map_err(|e| { tracing::error!("Error encoding value: {}", e); format!("Error encoding value: {}", e) }) }Then use it as:
let encoded_query = encode_to_vec(&end_time_query)?; let verification_path_query_bytes = encode_to_vec(&path_query)?;src/components/core_zmq_listener.rs (2)
Line range hint
31-35: Typo in enum variant nameChainLockedLockedTransactionThe variant
ChainLockedLockedTransactionappears to have a redundant 'Locked' in its name. Consider renaming it toChainLockedTransactionfor clarity and consistency.Apply this diff to fix the typo:
pub enum ZMQMessage { ISLockedTransaction(Transaction, InstantLock), ChainLockedBlock(Block), - ChainLockedLockedTransaction(Transaction, CoreBlockHeight), + ChainLockedTransaction(Transaction, CoreBlockHeight), }
Line range hint
71-71: Consider using a logging library instead ofprintln!andeprintln!Currently, the code uses
println!andeprintln!for logging information and errors. Consider using a logging library such aslogorenv_loggerwith appropriate logging macros (info!,warn!,error!) for better control over logging output and integration with logging frameworks.Example of replacing
println!andeprintln!with logging macros:-println!("Subscribed to ZMQ at {}", endpoint); +log::info!("Subscribed to ZMQ at {}", endpoint); -eprintln!("Error receiving message: {}", e); +log::error!("Error receiving message: {}", e);Don't forget to initialize the logger in your main function:
env_logger::init();Also applies to: 243-244, 251-252
🛑 Comments failed to post (4)
src/backend_task/contested_names/query_dpns_vote_contenders.rs (2)
74-112:
⚠️ Potential issueEnsure sensitive data is handled securely when logging
Be cautious when logging detailed error information that may include sensitive data like
proof_bytesor encoded queries.Consider redacting or masking sensitive information before logging or storing it to adhere to security best practices and protect against potential data leaks.
115-119: 🛠️ Refactor suggestion
Use error variants instead of matching on error messages
Matching on error message strings can be fragile and may break if the error messages change. It's better to match on specific error types to handle errors more reliably.
You can refactor this section to match on specific error variants:
-let error_str = e.to_string(); -if error_str.contains("try another server") - || error_str.contains( - "contract not found when querying from value with contract info", - ) +if matches!(e, dash_sdk::Error::Transient(_)) || matches!(e, dash_sdk::Error::ContractNotFound(_)) { retries += 1; if retries > MAX_RETRIES { tracing::error!( "Max retries reached for query_dpns_vote_contenders: {}", e ); return Err(format!( "Error fetching vote contenders after retries: {}", e )); } else { continue; } } else { // For other errors, return immediately return Err(format!("Error fetching vote contenders: {}", e)); }Committable suggestion skipped: line range outside the PR's diff.
src/backend_task/contested_names/query_ending_times.rs (2)
45-68:
⚠️ Potential issuePrevent potential runtime panic by safely handling enum variants
The current use of pattern matching assumes that every
vote_pollis aContestedDocumentResourceVotePoll. Ifvote_pollis of a different variant, this will cause a panic at runtime.To prevent this, consider using an
if letormatchstatement to safely destructurevote_poll. For example:if let VotePoll::ContestedDocumentResourceVotePoll( ContestedDocumentResourceVotePoll { contract_id, document_type_name, index_name, index_values, }, ) = vote_poll { // Proceed with processing // ... } else { // Handle other variants or skip return None; }
126-142: 🛠️ Refactor suggestion
Avoid string matching on errors; match on specific error types instead
Using
e.to_string().contains(...)to identify error types by their message is fragile and can lead to incorrect error handling if the error messages change.Consider matching on specific error variants to make the error handling more robust. For example:
if matches!(e, dash_sdk::Error::WrongNetwork) || matches!(e, dash_sdk::Error::ContractNotFound { .. }) { retries += 1; if retries >= MAX_RETRIES { tracing::error!("Max retries reached for query: {}", e); return Err(format!("Error fetching vote polls after retries: {}", e)); } else { continue; } } else { return Err(format!("Error fetching vote polls: {}", e)); }
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Insert proof logs into the db for dpns end time query and dpns vote contender query, and also add retry logic to end time query.
Summary by CodeRabbit
Release Notes
New Features
UI Improvements
Documentation