fix: error messages in dpns screen #72
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes in this pull request enhance error handling in the Changes
Possibly related PRs
Suggested reviewers
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
🧹 Outside diff range and nitpick comments (5)
src/ui/dpns_contested_names_screen.rs (1)
789-791: LGTM! Consider extracting the error message filter to a constant.The implementation correctly filters out transient error messages containing "try another server", which aligns with the PR objectives. This prevents unnecessary error notifications while the system attempts to retry the operation.
Consider extracting the error message filter to a constant for better maintainability:
impl DPNSContestedNamesScreen { + const FILTERED_ERROR_MESSAGES: [&'static str; 1] = ["try another server"]; + // ... other methods ... fn display_message(&mut self, message: &str, message_type: MessageType) { - if !message.contains("try another server") { + if !Self::FILTERED_ERROR_MESSAGES.iter().any(|&filter| message.contains(filter)) { self.error_message = Some((message.to_string(), message_type, Utc::now())); } }src/backend_task/contested_names/query_dpns_vote_contenders.rs (3)
80-80: Consider adding a delay between retriesCurrently, the loop retries immediately upon failure, which could lead to rapid successive retries and potential server overload. Adding a small delay between retries can help mitigate this issue and provide time for transient errors to resolve.
Insert the following line before
continue;at line 80:tokio::time::sleep(std::time::Duration::from_secs(1)).await;
47-86: Refactor retry logic into a reusable functionSince similar retry mechanisms are implemented in multiple methods, consider abstracting the retry logic into a reusable helper function. This will reduce code duplication and enhance maintainability.
62-63: Avoid logging errors multiple timesErrors are logged when fetching fails (
tracing::error!("Error fetching contested resources: {}", e);) and again when maximum retries are reached. This may result in redundant log messages. Consider adjusting the logging to avoid duplicate entries.src/backend_task/contested_names/query_dpns_contested_resources.rs (1)
Line range hint
41-115: Fix the retries counter to prevent an infinite loopThe
retriescounter is initialized inside the loop, causing it to reset to zero on each iteration. This means the conditionretries > MAX_RETRIESwill never be satisfied, potentially leading to an infinite loop if the error persists.Apply this diff to move the retries counter initialization outside the loop:
const MAX_RETRIES: usize = 3; + // Initialize retry counter + let mut retries = 0; let mut start_at_value = None; - // Initialize retry counter - let mut retries = 0; loop {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/backend_task/contested_names/query_dpns_contested_resources.rs(6 hunks)src/backend_task/contested_names/query_dpns_vote_contenders.rs(1 hunks)src/ui/dpns_contested_names_screen.rs(1 hunks)
| ) | ||
| { | ||
| retries += 1; | ||
| if retries > MAX_RETRIES { |
There was a problem hiding this comment.
Fix off-by-one error in retries condition
The condition if retries > MAX_RETRIES may allow for one additional retry beyond the intended MAX_RETRIES. Since retries is incremented before the check and starts at 0, the condition should be if retries >= MAX_RETRIES to ensure the method retries exactly MAX_RETRIES times.
Apply this change:
- if retries > MAX_RETRIES {
+ if retries >= MAX_RETRIES {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if retries > MAX_RETRIES { | |
| if retries >= MAX_RETRIES { |
| return Err(e); | ||
| } | ||
| } | ||
| format!("error fetching contested resources: {}", e) | ||
| })?; | ||
|
|
||
| if e.to_string().contains("try another server") | ||
| || e.to_string().contains( | ||
| "contract not found when querying from value with contract info", | ||
| ) | ||
| { | ||
| retries += 1; | ||
| 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 { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use specific error types instead of string matching for robust error handling
Matching errors using string comparisons like e.to_string().contains(...) is fragile and may break if error messages change. Consider matching on specific error types or creating custom error variants to make the error handling more robust.
Apply this diff to match on specific error variants:
- if e.to_string().contains("try another server")
- || e.to_string().contains(
- "contract not found when querying from value with contract info",
- )
+ if matches!(e,
+ dash_sdk::Error::NetworkError(_) |
+ dash_sdk::Error::ContractNotFound(_)
+ )This change assumes that dash_sdk::Error has specific variants for these error conditions.
Committable suggestion skipped: line range outside the PR's diff.
ogabrielides
left a comment
ogabrielides
left a comment
There was a problem hiding this comment.
rebase and fix conflicts. besides that LGTM
pauldelucia
force-pushed
the
fix/error-messages-in-dpns-screen
branch
from
42693b0 to
6c8f34c
Compare
Retry querying contested resources and contenders up to 3 times when error message contains "try another server" or "contract not found when querying from value with contract info".
Now, all contested names and contenders are fetched by just clicking "refresh" once, and there are no error messages at the top.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation