Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v0.2-dev into master #32

Draft
wants to merge 18 commits into
base: master
Choose a base branch
from
Draft

v0.2-dev into master #32

wants to merge 18 commits into from

Conversation

ogabrielides
Copy link
Collaborator

@ogabrielides ogabrielides commented Nov 1, 2024

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced a new WithdrawsStatusScreen for displaying withdrawal statuses with sorting and filtering options.
    • Added functionality for managing DPNS subscreens: Active, Past, and Owned.
    • Enhanced user data management with dynamic file paths for environment and log files.
  • Improvements

    • Improved organization of contest-related screens and added a new screen for withdrawal statuses.
    • Enhanced the IdentitiesScreen to limit displayed keys with an option to view more.
  • Bug Fixes

    • Updated error handling for loading configuration files and user data directories.

These changes enhance the application's user interface and functionality, providing a more robust experience for managing identities and withdrawals.

pauldelucia and others added 18 commits October 25, 2024 20:17
* feat: dpns subscreens

* finalized at a high level

* implement display for DPNSSubscreen

* more efficient filter
* work

* suggestions

* fmt

* refactoring

* suggestions

* sorting

* general info grid

* more work

* pagination

* fmt

* suggestions

* custom items per page

* Delete testnet_nodes.yml
* feat: view more keys pop-up in identities screen

* close pop-up on screen refresh

* fix: remove clone implementation for identities screen

* fixes

* put back commented out functionality

* cleanup

* fmt

---------

Co-authored-by: Quantum Explorer <[email protected]>
* auto-refresh on clicks and on startup

* various ui improvments

* rename

* clean up

* more work

* refactoring

* more work

* more work

* more work

---------

Co-authored-by: Quantum Explorer <[email protected]>
* feat: app data dir logic

* fmt

* suggestion

* suggestions

* renamed

* empty org
Copy link
Contributor

coderabbitai bot commented Nov 1, 2024

Walkthrough

The pull request introduces several changes across multiple files, primarily focusing on enhancing the application’s user interface and functionality related to withdrawal statuses and Domain Name Service (DPNS) management. Key updates include the addition of new modules, dependencies, and structural modifications to existing components, such as the AppState, QualifiedIdentity, and various UI screens. The changes also involve improved error handling and dynamic path management for user data and log files.

Changes

File Change Summary
Cargo.toml - Added new dependency: directories = "5.0"
- Updated serde_yaml to version 0.9.34+deprecated
src/app.rs - Updated AppState::new to include user data directory handling.
- Modified database initialization to use dynamic paths.
- Enhanced screen management for contests.
src/app_dir.rs - Added functions for managing user data directories and files, including path retrieval and environment file copying.
src/config.rs - Imported app_user_data_file_path and updated the load method to use it for .env file path handling.
src/context.rs - Added withdraws_contract field to AppContext and a method for retrieving local DPNS names.
src/logging.rs - Updated initialize_logger to use dynamic log file paths instead of hardcoded strings.
src/main.rs - Introduced the app_dir module and changed application title from "Identity Manager" to "Dash Evo Tool".
src/model/qualified_identity.rs - Added DPNSNameInfo struct and updated QualifiedIdentity to include a vector of DPNSNameInfo.
src/platform/identity/load_identity.rs - Enhanced load_identity to fetch and store DPNS names associated with identities.
src/platform/identity/register_identity.rs - Added dpns_names field to QualifiedIdentity structure.
src/platform/mod.rs - Introduced a new module for withdrawals and updated task handling to include withdrawal tasks.
src/platform/withdrawals/mod.rs - Added structures and methods for managing withdrawal transactions, including querying and data transformation.
src/ui/components/dpns_subscreen_chooser_panel.rs - Introduced a function for creating a DPNS subscreen chooser panel in the UI.
src/ui/components/left_panel.rs - Updated button configurations in the left panel for navigation to new screens.
src/ui/components/mod.rs - Added new module declaration for dpns_subscreen_chooser_panel.
src/ui/dpns_contested_names_screen.rs - Modified DPNSContestedNamesScreen to handle multiple subscreens and updated rendering logic accordingly.
src/ui/identities/identities_screen.rs - Added functionality to manage the display of additional keys in the identities screen.
src/ui/mod.rs - Added new screen module for WithdrawsStatusScreen and updated RootScreenType to include new variants.
src/ui/withdraws_status_screen.rs - Introduced WithdrawsStatusScreen for managing withdrawal statuses with methods for sorting, filtering, and displaying data.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant App
    participant AppContext
    participant WithdrawsStatusScreen

    User->>App: Open Withdraws Status Screen
    App->>AppContext: Retrieve withdrawal data
    AppContext->>WithdrawsStatusScreen: Load withdrawal records
    WithdrawsStatusScreen->>User: Display withdrawal data
Loading

Possibly related PRs

Suggested reviewers

  • QuantumExplorer
  • pauldelucia

Poem

🐇 In the garden of code, we hop and play,
With screens and states, we pave the way.
New paths for data, like carrots in rows,
Withdrawal statuses bloom, as the user knows.
With every update, our app takes flight,
Hopping through features, oh what a sight! 🌼


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 24

🧹 Outside diff range and nitpick comments (15)
src/app_dir.rs (1)

5-7: Document the empty constants and consider adding organization identifier.

Empty QUALIFIER and ORGANIZATION constants might affect directory structure clarity. While empty qualifier is common for Unix systems, consider:

  1. Adding documentation explaining why these are empty
  2. Setting an organization identifier to ensure proper directory hierarchy
-const QUALIFIER: &str = ""; // Typically empty on macOS and Linux
-const ORGANIZATION: &str = "";
+/// Qualifier for the application directory structure.
+/// Empty string is typical for macOS and Linux systems.
+const QUALIFIER: &str = "";
+
+/// Organization identifier for directory structure.
+/// Consider setting this to ensure proper directory hierarchy.
+const ORGANIZATION: &str = "dash";
src/ui/components/dpns_subscreen_chooser_panel.rs (1)

47-47: Remove unreachable catch-all pattern.

The _ pattern is unreachable as all variants of DPNSSubscreen are already handled in the previous match arms.

-                            _ => {}
src/platform/mod.rs (1)

78-80: Improve error handling and maintain consistent formatting.

The match arm implementation could be enhanced:

  1. Consider adding error context when calling run_withdraws_task
  2. The indentation is inconsistent with other match arms

Apply this diff to improve the implementation:

-            BackendTask::WithdrawalTask(withdrawal_task) => {
-                self.run_withdraws_task(withdrawal_task, &sdk).await
-            }
+            BackendTask::WithdrawalTask(withdrawal_task) => {
+                self.run_withdraws_task(withdrawal_task, &sdk)
+                    .await
+                    .map_err(|e| format!("Failed to process withdrawal task: {}", e))
+            }
src/context.rs (1)

60-62: Consider improving error handling for contract loading.

While the initialization follows the correct pattern, consider:

  1. Using a more descriptive error message that explains the impact of the failure
  2. Handling the error gracefully instead of using expect which could crash the application

Consider this improvement:

-        let withdrawal_contract =
-            load_system_data_contract(SystemDataContract::Withdrawals, PlatformVersion::latest())
-                .expect("expected to get withdrawal contract");
+        let withdrawal_contract = load_system_data_contract(
+            SystemDataContract::Withdrawals,
+            PlatformVersion::latest(),
+        ).map_err(|e| {
+            eprintln!("Failed to load withdrawals contract: {e}. Application functionality may be limited.");
+            e
+        })?;
src/model/qualified_identity.rs (2)

67-71: Add documentation and consider type safety improvements.

The new DPNSNameInfo struct would benefit from:

  1. Documentation explaining its purpose and field meanings
  2. A newtype wrapper for the timestamp to ensure type safety

Consider this improvement:

+/// Represents a Domain Name Service (DPNS) name registration
+/// associated with an identity.
 #[derive(Debug, Encode, Decode, Clone, PartialEq)]
 pub struct DPNSNameInfo {
+    /// The registered DPNS name
     pub name: String,
+    /// Unix timestamp (in seconds) when the name was acquired
     pub acquired_at: u64,
 }

Also consider creating a newtype for the timestamp:

/// Represents a Unix timestamp in seconds
#[derive(Debug, Encode, Decode, Clone, PartialEq)]
pub struct Timestamp(pub u64);

281-281: Consider using Default trait.

The empty vector initialization could leverage the Default trait.

- dpns_names: vec![],
+ dpns_names: Default::default(),
src/ui/identities/identities_screen.rs (3)

59-70: LGTM with suggestion: Consider extracting magic number.

The ID truncation logic is well-implemented, but consider extracting the number 8 as a named constant for better maintainability.

+ const ID_DISPLAY_LENGTH: usize = 8;
+
  let identifier_as_string_first_8_chars: String = qualified_identity
      .identity
      .id()
      .to_string(encoding)
      .chars()
-     .take(8)
+     .take(ID_DISPLAY_LENGTH)
      .collect();

130-179: LGTM with suggestion: Consider extracting text content.

The UI layout and styling are well-structured with good visual hierarchy. Consider extracting the text content to constants or a localization file for easier maintenance and potential internationalization.

const NO_IDENTITIES_HEADING: &str = "Not Tracking Any Identities";
const NO_IDENTITIES_DESCRIPTION: &str = "It looks like you are not tracking any Identities, Evonodes or Masternodes yet.";
// ... other text constants

271-334: LGTM with suggestions: Consider optimizations and constant extraction.

The key display logic is well-implemented but could benefit from some refinements:

  1. Extract magic numbers as constants
  2. Consider pre-calculating total key count to avoid unnecessary iterations
+ const MAX_KEYS_TO_SHOW: usize = 3;
+ 
- let max_keys_to_show = 3;
+ let max_keys_to_show = MAX_KEYS_TO_SHOW;

+ // Pre-calculate total keys for efficiency
+ let total_keys = public_keys.len() + 
+     voter_identity_public_keys.map_or(0, |keys| keys.len());
+ let more_keys_available = total_keys > max_keys_to_show;
src/ui/mod.rs (1)

Line range hint 292-292: Correct the mapping in screen_type for AddNewIdentityScreen

In the screen_type method, the mapping for Screen::AddNewIdentityScreen(_) incorrectly returns ScreenType::AddExistingIdentity. It should return ScreenType::AddNewIdentity to accurately represent the screen type.

Apply this diff to fix the mapping:

-                Screen::AddNewIdentityScreen(_) => ScreenType::AddExistingIdentity,
+                Screen::AddNewIdentityScreen(_) => ScreenType::AddNewIdentity,
src/platform/withdrawals/mod.rs (1)

217-223: Simplify error handling when retrieving sum tree value.

The error message "could not get sum tree value for current withdrawal maximum" is repeated multiple times. Refactor the error handling to avoid duplication and improve code clarity.

You can combine the checks or extract the error message into a constant:

+const ERR_SUM_TREE_VALUE: &str = "could not get sum tree value for current withdrawal maximum";

Update the error handling:

-                    .ok_or_else(|| {
-                        "could not get sum tree value for current withdrawal maximum".to_string()
-                    })?;
+                    .ok_or_else(|| ERR_SUM_TREE_VALUE.to_string())?;
src/app.rs (1)

14-14: Consider renaming WithdrawsStatusScreen to WithdrawalStatusScreen

The term "Withdraws" may not be grammatically correct in this context. Consider renaming WithdrawsStatusScreen to WithdrawalStatusScreen for better readability and consistency.

Apply this diff to rename the import:

-use crate::ui::withdraws_status_screen::WithdrawsStatusScreen;
+use crate::ui::withdrawal_status_screen::WithdrawalStatusScreen;

Additionally, rename the file withdraws_status_screen.rs to withdrawal_status_screen.rs, and update all occurrences of WithdrawsStatusScreen in the codebase accordingly.

src/ui/withdraws_status_screen.rs (1)

313-313: Remove unnecessary parentheses in 'if' condition

In Rust, parentheses around if conditions are not required and can be omitted for cleaner code.

Apply this diff to remove the unnecessary parentheses:

-if (total_pages > 0) {
+if total_pages > 0 {
src/ui/dpns_contested_names_screen.rs (2)

Line range hint 240-416: Refactor Duplicate Code in Table Rendering Functions

The functions render_table_active_contests and render_table_past_contests have significant overlap in their structure and logic, leading to code duplication.

Consider refactoring by abstracting the common logic into a generic function or using helper methods. This will improve maintainability and make future updates easier. For example:

  • Create a generic function that accepts data and configuration parameters to render the table.
  • Extract repeated code segments into separate functions that can be reused.

119-126: Handle Potential Short IDs Gracefully

When formatting the first_6_chars_of_id, you take the first 6 characters of the contestant.id. If the ID is shorter than 6 characters, this could lead to unexpected formatting.

Consider adjusting the code to handle IDs shorter than 6 characters:

-    let first_6_chars_of_id: String = contestant
-        .id
-        .to_string(Encoding::Base58)
-        .chars()
-        .take(6)
-        .collect();
+    let id_str = contestant.id.to_string(Encoding::Base58);
+    let first_6_chars_of_id = if id_str.len() >= 6 {
+        &id_str[..6]
+    } else {
+        &id_str
+    };
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between a29902d and 0ffa920.

⛔ Files ignored due to path filters (1)
  • icons/withdraws.png is excluded by !**/*.png
📒 Files selected for processing (19)
  • Cargo.toml (1 hunks)
  • src/app.rs (7 hunks)
  • src/app_dir.rs (1 hunks)
  • src/config.rs (2 hunks)
  • src/context.rs (5 hunks)
  • src/logging.rs (1 hunks)
  • src/main.rs (2 hunks)
  • src/model/qualified_identity.rs (3 hunks)
  • src/platform/identity/load_identity.rs (3 hunks)
  • src/platform/identity/register_identity.rs (1 hunks)
  • src/platform/mod.rs (5 hunks)
  • src/platform/withdrawals/mod.rs (1 hunks)
  • src/ui/components/dpns_subscreen_chooser_panel.rs (1 hunks)
  • src/ui/components/left_panel.rs (2 hunks)
  • src/ui/components/mod.rs (1 hunks)
  • src/ui/dpns_contested_names_screen.rs (14 hunks)
  • src/ui/identities/identities_screen.rs (10 hunks)
  • src/ui/mod.rs (12 hunks)
  • src/ui/withdraws_status_screen.rs (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • src/ui/components/mod.rs
🔇 Additional comments (47)
src/main.rs (2)

31-31: LGTM! Application title updated.

The window title change from "Identity Manager" to "Dash Evo Tool" is consistent with the PR's UI enhancements.

Let's verify this is the only instance of the old title:

#!/bin/bash
# Description: Check for any remaining instances of the old title
# Expected: No instances of "Identity Manager" should remain

echo "Checking for old application title..."
rg "Identity Manager" -l

Line range hint 8-13: LGTM! Verify new module files exist.

The new module declarations look good and align with the broader restructuring described in the summary.

Let's verify the existence of these new module files:

✅ Verification successful

All module declarations are properly implemented

All declared modules in src/main.rs have corresponding implementations:

  • app_dir.rs, components.rs, context.rs, and context_provider.rs exist as direct files
  • model and platform are properly structured directories with mod.rs files and submodules
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all newly imported modules exist as Rust files
# Expected: All files should exist either as .rs files or as mod.rs within directories

echo "Checking for module files..."
for module in app_dir components context context_provider model platform; do
    fd -e rs "^${module}.rs$|^${module}/mod.rs$" src/
done

Length of output: 925


Script:

#!/bin/bash
# Let's check for model and platform modules more thoroughly
# They might be directories with mod.rs files inside

# Check for model directory and its contents
echo "Checking model directory..."
fd . src/model/

# Check for platform directory and its contents
echo "Checking platform directory..."
fd . src/platform/

# Also check if these are maybe defined in lib.rs or other common module files
echo "Checking other possible module locations..."
rg "^mod (model|platform)" src/

Length of output: 1325

Cargo.toml (2)

44-44: LGTM: Good choice of the directories crate.

The addition of the directories crate is appropriate for cross-platform filesystem operations and aligns well with the PR's objectives for managing user data directories.


47-47: ⚠️ Potential issue

Warning: Investigate the deprecated serde_yaml version.

The serde_yaml = "0.9.34+deprecated" dependency version has a concerning "+deprecated" suffix. This could lead to future compatibility issues or security vulnerabilities.

Consider updating to the latest stable version:

-serde_yaml = "0.9.34+deprecated"
+serde_yaml = "0.9"
⛔ Skipped due to learnings
Learnt from: ogabrielides
PR: dashpay/dash-evo-tool#22
File: Cargo.toml:47-47
Timestamp: 2024-10-29T09:24:57.158Z
Learning: In `Cargo.toml`, the `serde_yaml` dependency is intentionally set to `"0.9.34+deprecated"`, and updating it is not necessary unless specifically required.
src/logging.rs (2)

1-1: LGTM! Clean import of the new dependency.

The explicit import from the local crate is well-structured.


8-12: 🛠️ Refactor suggestion

Consider more graceful error handling for logger initialization.

The current implementation has several areas that could be improved:

  1. Using expect for path creation could lead to abrupt termination. Consider returning Result instead.
  2. Error messages could be more descriptive to aid in troubleshooting.
  3. No cleanup handling for partially created resources.

Consider refactoring to handle errors more gracefully:

-pub fn initialize_logger() {
+pub fn initialize_logger() -> Result<(), Box<dyn std::error::Error>> {
     // Initialize log file, with improved error handling
-    let log_file_path = app_user_data_file_path("explorer.log").expect("should create log file path");
-    let log_file = match std::fs::File::create(log_file_path)
-    {
-        Ok(file) => file,
-        Err(e) => panic!("Failed to create log file: {:?}", e),
-    };
+    let log_file_path = app_user_data_file_path("explorer.log")
+        .map_err(|e| format!("Failed to create log file path: {}", e))?;
+    
+    let log_file = std::fs::File::create(&log_file_path)
+        .map_err(|e| format!("Failed to create log file at {:?}: {}", log_file_path, e))?;

Let's verify if this logging initialization is used in critical paths:

src/app_dir.rs (1)

9-16: 🛠️ Refactor suggestion

Consider using data_dir() instead of config_dir() for user data.

The function returns the config directory path, but since this is for user data, data_dir() might be more appropriate. Config directories are typically used for application settings, while data directories are meant for user data.

-    Ok(proj_dirs.config_dir().to_path_buf())
+    Ok(proj_dirs.data_dir().to_path_buf())
src/ui/components/dpns_subscreen_chooser_panel.rs (2)

1-3: LGTM! Clean and well-organized imports.

The imports are minimal, well-organized, and properly scoped.


5-57: Overall implementation is clean and well-structured.

The panel implementation follows good UI patterns and provides clear navigation between DPNS subscreens. While there are some opportunities for optimization, the core functionality is solid and maintainable.

src/platform/mod.rs (4)

8-8: LGTM!

The import statement is properly structured and imports the necessary types from the new withdrawals module.


28-28: LGTM!

The new WithdrawalTask variant follows the established pattern and maintains consistency with other task types.


38-38: Consider implications of partial data handling.

While the variant is correctly implemented, the use of WithdrawStatusPartialData suggests incomplete data. Ensure proper error handling and UI feedback when complete data is required but only partial data is available.

✅ Verification successful

Let me gather more information about the error handling and data conversion.


Partial data handling is properly implemented

The implementation has robust error handling for partial data:

  • The TryFrom trait implementation properly handles missing fields with clear error messages
  • The UI component (withdraws_status_screen.rs) uses thread-safe Arc<RwLock<Option<>>> wrapper indicating safe handling of potentially missing data
  • A merge_with_data function exists to properly combine partial data updates
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check error handling for partial data
# Expected: Find error handling patterns for partial data

# Look for error handling related to partial data
rg -A 5 "WithdrawStatusPartialData"

Length of output: 3043


Script:

#!/bin/bash
# Check for error handling in WithdrawStatusData conversion and usage
ast-grep --pattern 'impl TryFrom<WithdrawStatusPartialData> for WithdrawStatusData {
  $$$
}'

# Look for WithdrawStatusData usage and error handling
rg -A 3 "WithdrawStatusData" 

Length of output: 3837


19-19: Verify the public interface of the withdrawals module.

The module is correctly declared as public. However, since this is a new module, ensure that only the necessary types and functions are exposed publicly.

✅ Verification successful

Public interface is well-structured and appropriate

The withdrawals module's public interface is well-designed with clear separation of concerns:

  • Public types (WithdrawalsPerPage, StartAfterIdentifier) are used for API parameters
  • Data structures (WithdrawRecord, WithdrawStatusData) expose only necessary fields
  • Internal implementation details are properly hidden with pub(crate) and private functions
  • Public constants and error types are appropriately scoped

The public interface aligns well with the module's purpose of handling withdrawal functionality.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check the public interface of the withdrawals module
# Expected: Only necessary types should be public

# Check for public declarations in the withdrawals module
ast-grep --pattern 'pub $$$' src/platform/withdrawals/mod.rs

Length of output: 62


Script:

#!/bin/bash
# Let's first check if the file exists and see its content
fd -t f "withdrawals/mod.rs" src/platform --exec cat {}

# Also search for any Rust files in the withdrawals module
fd -t f -e rs "." src/platform/withdrawals/

# As a backup, let's also check for any public declarations in all Rust files under withdrawals
fd -t f -e rs "." src/platform/withdrawals/ --exec ast-grep --pattern 'pub $$$' {}

Length of output: 218


Script:

#!/bin/bash
# Let's try to read the content of the file using ripgrep which might handle the file better
rg --no-line-number "" src/platform/withdrawals/mod.rs

# Also search for any struct, enum, or function declarations to understand the module's interface
rg -A 3 "^(pub |pub\(.*\) )*(struct|enum|fn|type|trait|const|static|mod)" src/platform/withdrawals/mod.rs

Length of output: 12867

src/ui/components/left_panel.rs (2)

59-63: Verify withdraws.png icon exists in embedded assets.

The new button addition follows the established pattern and is well-placed in the navigation flow. However, let's ensure the required icon file exists.

#!/bin/bash
# Description: Verify the existence of the withdraws icon and screen implementation

# Test 1: Check if the withdraws.png exists in the icons directory
echo "Checking for withdraws.png..."
fd "withdraws.png" --type f

# Test 2: Verify the WithdrawsStatus screen implementation
echo "Checking WithdrawsStatus screen implementation..."
rg "RootScreenWithdrawsStatus" -A 5

50-50: Verify DPNS screen navigation consistency.

The button destination has been updated from RootScreenDPNSContestedNames to RootScreenDPNSActiveContests. Let's ensure this change is consistently applied across the codebase.

✅ Verification successful

Navigation change is consistent across the codebase

The change from RootScreenDPNSContestedNames to RootScreenDPNSActiveContests has been consistently implemented throughout the codebase. The search results show that:

  • The new screen type RootScreenDPNSActiveContests is properly defined in the RootScreenType enum
  • It's correctly mapped in screen type conversions and integer mappings
  • It's used consistently across multiple files including app.rs, ui/mod.rs, dpns_contested_names_screen.rs, and dpns_subscreen_chooser_panel.rs
  • No references to the old screen type remain in the codebase
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining references to the old screen type and verify navigation flow

# Test 1: Search for any remaining references to the old screen type
echo "Checking for old screen type references..."
rg "RootScreenDPNSContestedNames"

# Test 2: Verify the new screen type implementation
echo "Checking new screen type implementation..."
rg "RootScreenDPNSActiveContests" -A 5

Length of output: 2469


Script:

#!/bin/bash
# Let's search specifically for both old and new DPNS screen types
echo "Checking for DPNS screen types..."
rg "RootScreenDPNS" -A 2

# Also check for any enum definitions containing these screen types
echo "Checking enum definitions..."
ast-grep --pattern 'enum RootScreenType {
  $$$
}'

Length of output: 4043

src/config.rs (2)

3-3: LGTM: Clean import addition

The new import is well-placed and follows Rust naming conventions.


63-64: Verify test coverage for platform-specific path handling

Since this code handles file paths which can be platform-dependent, we should ensure proper test coverage exists.

src/platform/identity/register_identity.rs (2)

160-160: Consider updating documentation for the new DPNS names field.

Since this is a significant structural change adding DPNS support, ensure that any relevant documentation (API docs, README, etc.) is updated to reflect this new capability.

#!/bin/bash
# Description: Check for documentation files that might need updating

# Look for documentation files
fd -e md -e rst -e txt --exec rg -l "identity|DPNS|DNS" {}

160-160: Verify DPNS names field usage across the codebase.

The initialization of dpns_names as an empty vector is correct for new identity registration. However, let's ensure this change is properly integrated.

✅ Verification successful

The empty initialization of dpns_names is correctly integrated across the codebase

The verification shows that:

  • The dpns_names field is properly defined in QualifiedIdentity struct and used consistently
  • The field is accessed appropriately in context.rs for iterating over DPNS names
  • load_identity.rs handles the population of DPNS names correctly
  • The empty initialization during registration is appropriate as names are managed separately through the DPNS system
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of dpns_names field across the codebase

# Check for any direct field access
rg -A 2 "dpns_names" 

# Check for any DPNS-related operations that might need updating
ast-grep --pattern 'struct $name { $$$ dpns_names: $type $$$ }'

Length of output: 244


Script:

#!/bin/bash
# Let's try a more focused search approach

# Search for struct definitions containing dpns_names
rg -U "struct.*\{[\s\S]*dpns_names.*\}" 

# Search for direct usage of dpns_names field
rg "\.dpns_names" 

# Search for any DPNS related types or imports
rg "DPNSName"

# Search specifically in the identity module
fd -t f -e rs . src/platform/identity/ --exec rg -l "dpns"

Length of output: 3371

src/context.rs (3)

6-6: LGTM: Import statements are properly organized.

The new imports are correctly placed and align with the added functionality.

Also applies to: 12-12, 16-16


31-31: LGTM: Field addition follows established patterns.

The withdraws_contract field correctly uses Arc for shared ownership and maintains consistent visibility with other system contracts.


92-92: LGTM: Consistent initialization pattern.

The withdraws_contract field is properly initialized following the same pattern as other system contracts.

src/platform/identity/load_identity.rs (3)

5-5: LGTM: New imports are well-organized and necessary.

The added imports support the new DPNS name fetching functionality.

Also applies to: 9-9, 14-16


200-200: LGTM: QualifiedIdentity struct update.

The dpns_names field is correctly initialized with the fetched DPNS names.


144-156: Consider pagination and sorting for DPNS name fetching.

The current implementation has several potential limitations:

  1. The hardcoded limit of 100 might truncate results for users with many DPNS names.
  2. No pagination implementation despite having a limit.
  3. Lack of sorting could lead to inconsistent ordering across queries.

Let's check if there are any users with more than 100 DPNS names:

src/model/qualified_identity.rs (1)

67-71: Verify integration with DPNS-related functionality.

The addition of DPNS name storage capabilities might require updates in related components.

Also applies to: 83-83, 281-281

✅ Verification successful

Based on the search results, I can now generate the final response since I have comprehensive information about the usage of DPNSNameInfo and QualifiedIdentity in the codebase.

No breaking changes detected in DPNS-related functionality

The DPNSNameInfo struct is properly integrated within the codebase:

  • It's used consistently in DPNS-related components like dpns_contested_names_screen.rs
  • The struct is properly serialized/deserialized through the QualifiedIdentity implementation
  • All references to the struct maintain the same field structure (name and acquired_at)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for DPNS-related code that might need updates

# Look for DPNS-related files and functionality
echo "Searching for DPNS-related files..."
rg -l "DPNS|dpns" --type rust

# Check for potential uses of QualifiedIdentity that might need updates
echo "Checking QualifiedIdentity usage..."
rg -A 3 "QualifiedIdentity" --type rust

# Look for serialization/deserialization of QualifiedIdentity
echo "Checking serialization usage..."
rg -A 3 "to_bytes|from_bytes.*QualifiedIdentity" --type rust

Length of output: 24230

src/ui/identities/identities_screen.rs (3)

29-29: LGTM: Well-designed state management for popup.

The Option<QualifiedIdentity> type is appropriate for managing the popup state, allowing for clear presence/absence semantics.


33-44: LGTM: Constructor properly initializes all fields.

The constructor correctly initializes the new show_more_keys_popup field to None and handles potential errors when loading identities.


Line range hint 439-496: LGTM: Clean state management in refresh and UI methods.

The refresh method properly cleans up the popup state, and the UI method correctly handles the popup window display.

src/ui/mod.rs (6)

Line range hint 45-65: New RootScreenType variants and mappings are correctly implemented

The additions of RootScreenDPNSActiveContests, RootScreenDPNSPastContests, RootScreenDPNSOwnedNames, and RootScreenWithdrawsStatus to the RootScreenType enum are properly reflected in the to_int and from_int methods. The integer mappings are consistent and correctly updated.

Also applies to: 73-79


Line range hint 106-119: New ScreenType variants added appropriately

The ScreenType enum now includes DPNSActiveContests, DPNSPastContests, DPNSMyUsernames, and WithdrawsStatus. These additions align with the new functionality and are correctly incorporated.


128-137: create_screen method correctly instantiates new screens

The create_screen method accurately constructs the new DPNS screens (DPNSActiveContests, DPNSPastContests, DPNSMyUsernames) with the appropriate DPNSSubscreen. It also properly handles the instantiation of the WithdrawsStatus screen.

Also applies to: 175-177


221-221: change_context method updated appropriately

The change_context method now correctly updates the application context for the new WithdrawsStatusScreen, ensuring consistent context management across all screens.


273-284: DPNS sub-screen mappings in screen_type are correct

The screen_type method correctly maps the DPNSContestedNamesScreen with different DPNSSubscreen variants (Active, Past, Owned) to the corresponding ScreenType variants. This ensures proper identification and handling of each DPNS subscreen.


296-296: WithdrawsStatusScreen mapping in screen_type is correct

The mapping of Screen::WithdrawsStatusScreen(_) to ScreenType::WithdrawsStatus in the screen_type method is accurate, ensuring the screen type is correctly identified.

src/app.rs (9)

1-4: Imports updated to include new functions

The new functions app_user_data_file_path, copy_env_file_if_not_exists, and create_app_user_data_directory_if_not_exists are correctly imported from crate::app_dir.


10-10: Imports updated to include DPNSSubscreen

The import statement now includes DPNSSubscreen, which is necessary for the new DPNS subscreens.


109-109: Database file path correctly obtained

The database file path is correctly obtained using app_user_data_file_path("data.db") with appropriate error handling.


127-132: Initialization of DPNS subscreens

The DPNS subscreens (Active, Past, and Owned) are correctly initialized with the mainnet_app_context and the appropriate DPNSSubscreen variants.


136-136: Initialization of WithdrawsStatusScreen

The withdraws_status_screen is correctly initialized with the mainnet_app_context.


153-158: Re-initialization of DPNS subscreens for testnet

When switching to testnet, the DPNS subscreens are re-initialized with the testnet_app_context, ensuring they function correctly on the test network.


161-161: Re-initialization of WithdrawsStatusScreen for testnet

The withdraws_status_screen is re-initialized with the testnet_app_context when switching networks, ensuring correct functionality.


Line range hint 179-201: Addition of new root screens to main_screens

New entries for RootScreenDPNSActiveContests, RootScreenDPNSPastContests, RootScreenDPNSOwnedNames, and RootScreenWithdrawsStatus are correctly added to the main_screens map with their corresponding screen instances.


314-316: Correctly handling WithdrawalStatus in task results

The update method now handles the BackendTaskSuccessResult::WithdrawalStatus(_) variant, ensuring that the application displays withdrawal status appropriately.

src/ui/withdraws_status_screen.rs (1)

384-405: ⚠️ Potential issue

Verify pagination logic for off-by-one errors

There may be a potential off-by-one error in the pagination controls. Ensure that the "Next" button does not advance beyond the last page, and the "Previous" button does not go below the first page.

Review the conditions for the pagination buttons:

  • Ensure that the "Next" button is disabled or does not increment the page number when on the last page.
  • Confirm that current_page does not exceed total_pages - 1.
  • Check that the "Previous" button does not decrement the page number below zero.
src/ui/dpns_contested_names_screen.rs (2)

58-61: Consider Using Arc for Shared Data Across Threads

The voting_identities and user_identities fields have been changed from Arc<Vec<QualifiedIdentity>> to Vec<QualifiedIdentity>. If these identities are accessed across multiple threads, removing Arc may lead to concurrency issues.

Ensure that the identities are not shared across threads or consider retaining Arc to manage shared ownership safely.


716-729: Ensure Consistent Data Refresh Across Subscreens

In the refresh method, when dpns_subscreen is DPNSSubscreen::Owned, only local_dpns_names is refreshed, whereas for other subscreens, contested_names is updated. This could lead to inconsistencies if contested_names needs to be refreshed for the Owned subscreen as well.

Review whether contested_names should also be refreshed when dpns_subscreen is Owned. Update the refresh method accordingly if needed.

Comment on lines +32 to +41
pub fn app_user_data_file_path(filename: &str) -> Result<PathBuf, std::io::Error> {
if filename.is_empty() || filename.contains(std::path::is_separator) {
return Err(std::io::Error::new(
std::io::ErrorKind::InvalidInput,
"Invalid filename",
));
}
let app_data_dir = app_user_data_dir_path()?;
Ok(app_data_dir.join(filename))
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance filename validation.

While the function checks for path separators, consider adding validation for:

  1. Maximum filename length
  2. Special characters that might cause issues
  3. Reserved filenames on Windows (e.g., CON, PRN, etc.)
+const MAX_FILENAME_LENGTH: usize = 255;
+
 pub fn app_user_data_file_path(filename: &str) -> Result<PathBuf, std::io::Error> {
-    if filename.is_empty() || filename.contains(std::path::is_separator) {
+    if filename.is_empty() 
+        || filename.contains(std::path::is_separator)
+        || filename.len() > MAX_FILENAME_LENGTH
+        || filename.chars().any(|c| c.is_control())
+        || cfg!(windows) && is_windows_reserved_filename(filename)
+    {
         return Err(std::io::Error::new(
             std::io::ErrorKind::InvalidInput,
-            "Invalid filename",
+            "Invalid filename: must not be empty, contain separators, exceed length limit, or be a reserved name",
         ));
     }

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +17 to +30
pub fn create_app_user_data_directory_if_not_exists() -> Result<(), std::io::Error> {
let app_data_dir = app_user_data_dir_path()?;
fs::create_dir_all(&app_data_dir)?;

// Verify directory permissions
let metadata = fs::metadata(&app_data_dir)?;
if !metadata.is_dir() {
return Err(std::io::Error::new(
std::io::ErrorKind::Other,
"Created path is not a directory",
));
}
Ok(())
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider adding permission checks for Unix systems.

While the function correctly creates and verifies the directory, it might be worth adding permission checks on Unix systems to ensure the directory is readable and writable by the current user.

     if !metadata.is_dir() {
         return Err(std::io::Error::new(
             std::io::ErrorKind::Other,
             "Created path is not a directory",
         ));
     }
+    #[cfg(unix)]
+    {
+        use std::os::unix::fs::PermissionsExt;
+        let perms = metadata.permissions();
+        if perms.mode() & 0o600 != 0o600 {
+            return Err(std::io::Error::new(
+                std::io::ErrorKind::PermissionDenied,
+                "Insufficient directory permissions",
+            ));
+        }
+    }
     Ok(())
📝 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.

Suggested change
pub fn create_app_user_data_directory_if_not_exists() -> Result<(), std::io::Error> {
let app_data_dir = app_user_data_dir_path()?;
fs::create_dir_all(&app_data_dir)?;
// Verify directory permissions
let metadata = fs::metadata(&app_data_dir)?;
if !metadata.is_dir() {
return Err(std::io::Error::new(
std::io::ErrorKind::Other,
"Created path is not a directory",
));
}
Ok(())
}
pub fn create_app_user_data_directory_if_not_exists() -> Result<(), std::io::Error> {
let app_data_dir = app_user_data_dir_path()?;
fs::create_dir_all(&app_data_dir)?;
// Verify directory permissions
let metadata = fs::metadata(&app_data_dir)?;
if !metadata.is_dir() {
return Err(std::io::Error::new(
std::io::ErrorKind::Other,
"Created path is not a directory",
));
}
#[cfg(unix)]
{
use std::os::unix::fs::PermissionsExt;
let perms = metadata.permissions();
if perms.mode() & 0o600 != 0o600 {
return Err(std::io::Error::new(
std::io::ErrorKind::PermissionDenied,
"Insufficient directory permissions",
));
}
}
Ok(())
}

Comment on lines +43 to +56
pub fn copy_env_file_if_not_exists() {
let app_data_dir = app_user_data_dir_path().expect("Failed to determine application data directory");
let env_file = app_data_dir.join(".env".to_string());
if env_file.exists() && env_file.is_file() {
} else {
let env_example_file = PathBuf::from(".env.example");
let target_env_file_path = app_user_data_file_path(".env").expect("should create target env file path");
fs::copy(
&env_example_file,
target_env_file_path,
)
.expect("Failed to copy main net env file");
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Critical: Improve error handling and security in env file copying.

Several issues need attention:

  1. Using expect() can cause panics - should return Result instead
  2. No validation if source .env.example exists
  3. Potential security risk with hardcoded paths
  4. No file content validation before copying

Here's a safer implementation:

-pub fn copy_env_file_if_not_exists() {
+pub fn copy_env_file_if_not_exists() -> Result<(), std::io::Error> {
     let app_data_dir = app_user_data_dir_path()
-        .expect("Failed to determine application data directory");
+        ?;
     let env_file = app_data_dir.join(".env");
     if env_file.exists() && env_file.is_file() {
+        return Ok(());
     } else {
         let env_example_file = PathBuf::from(".env.example");
-        let target_env_file_path = app_user_data_file_path(".env")
-            .expect("should create target env file path");
+        if !env_example_file.exists() {
+            return Err(std::io::Error::new(
+                std::io::ErrorKind::NotFound,
+                "Template .env.example file not found",
+            ));
+        }
+        let target_env_file_path = app_user_data_file_path(".env")?;
-        fs::copy(
+        fs::copy(
             &env_example_file,
             target_env_file_path,
-        )
-        .expect("Failed to copy main net env file");
+        )?;
     }
+    Ok(())
 }
📝 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.

Suggested change
pub fn copy_env_file_if_not_exists() {
let app_data_dir = app_user_data_dir_path().expect("Failed to determine application data directory");
let env_file = app_data_dir.join(".env".to_string());
if env_file.exists() && env_file.is_file() {
} else {
let env_example_file = PathBuf::from(".env.example");
let target_env_file_path = app_user_data_file_path(".env").expect("should create target env file path");
fs::copy(
&env_example_file,
target_env_file_path,
)
.expect("Failed to copy main net env file");
}
}
pub fn copy_env_file_if_not_exists() -> Result<(), std::io::Error> {
let app_data_dir = app_user_data_dir_path()?;
let env_file = app_data_dir.join(".env");
if env_file.exists() && env_file.is_file() {
return Ok(());
} else {
let env_example_file = PathBuf::from(".env.example");
if !env_example_file.exists() {
return Err(std::io::Error::new(
std::io::ErrorKind::NotFound,
"Template .env.example file not found",
));
}
let target_env_file_path = app_user_data_file_path(".env")?;
fs::copy(
&env_example_file,
target_env_file_path,
)?;
}
Ok(())
}

Comment on lines +8 to +12
let subscreens = vec![
DPNSSubscreen::Active,
DPNSSubscreen::Past,
DPNSSubscreen::Owned,
];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider using a constant for static subscreens.

Since the subscreens vector is static and doesn't change, consider defining it as a constant to avoid recreation on each function call.

const DPNS_SUBSCREENS: &[DPNSSubscreen] = &[
    DPNSSubscreen::Active,
    DPNSSubscreen::Past,
    DPNSSubscreen::Owned,
];

Then use it in the function:

-    let subscreens = vec![
-        DPNSSubscreen::Active,
-        DPNSSubscreen::Past,
-        DPNSSubscreen::Owned,
-    ];
+    let subscreens = DPNS_SUBSCREENS;

Comment on lines +14 to +20
SidePanel::left("dpns_subscreen_chooser_panel")
.default_width(250.0)
.frame(
Frame::none()
.fill(ctx.style().visuals.panel_fill)
.inner_margin(Margin::same(10.0)),
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider making panel width responsive.

The fixed width of 250.0 might not be optimal for all screen sizes. Consider making it responsive based on the context's available width.

-        .default_width(250.0)
+        .default_width(ctx.available_rect().width().min(250.0))
📝 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.

Suggested change
SidePanel::left("dpns_subscreen_chooser_panel")
.default_width(250.0)
.frame(
Frame::none()
.fill(ctx.style().visuals.panel_fill)
.inner_margin(Margin::same(10.0)),
)
SidePanel::left("dpns_subscreen_chooser_panel")
.default_width(ctx.available_rect().width().min(250.0))
.frame(
Frame::none()
.fill(ctx.style().visuals.panel_fill)
.inner_margin(Margin::same(10.0)),
)

Comment on lines +212 to +290
if ui.checkbox(&mut self.filter_status_queued, "Queued").changed() {
self.util_build_combined_filter_status_mix();
self.requested_data = true;
let mut lock_data = self.data.write().unwrap();
*lock_data = None;
app_action |= AppAction::BackendTask(BackendTask::WithdrawalTask(
WithdrawalsTask::QueryWithdrawals(
self.filter_status_mix.clone(),
self.pagination_items_per_page.into(),
None,
true,
true,
)
));
}
ui.add_space(8.0);
if ui.checkbox(&mut self.filter_status_pooled, "Pooled").changed() {
self.util_build_combined_filter_status_mix();
self.requested_data = true;
let mut lock_data = self.data.write().unwrap();
*lock_data = None;
app_action |= AppAction::BackendTask(BackendTask::WithdrawalTask(
WithdrawalsTask::QueryWithdrawals(
self.filter_status_mix.clone(),
self.pagination_items_per_page.into(),
None,
true,
true,
)
));
}
ui.add_space(8.0);
if ui.checkbox(&mut self.filter_status_broadcasted, "Broadcasted").changed() {
self.util_build_combined_filter_status_mix();
self.requested_data = true;
let mut lock_data = self.data.write().unwrap();
*lock_data = None;
app_action |= AppAction::BackendTask(BackendTask::WithdrawalTask(
WithdrawalsTask::QueryWithdrawals(
self.filter_status_mix.clone(),
self.pagination_items_per_page.into(),
None,
true,
true,
)
));
}
ui.add_space(8.0);
if ui.checkbox(&mut self.filter_status_complete, "Complete").changed() {
self.util_build_combined_filter_status_mix();
self.requested_data = true;
let mut lock_data = self.data.write().unwrap();
*lock_data = None;
app_action |= AppAction::BackendTask(BackendTask::WithdrawalTask(
WithdrawalsTask::QueryWithdrawals(
self.filter_status_mix.clone(),
self.pagination_items_per_page.into(),
None,
true,
true,
)
));
}
ui.add_space(8.0);
if ui.checkbox(&mut self.filter_status_expired, "Expired").changed() {
self.util_build_combined_filter_status_mix();
self.requested_data = true;
let mut lock_data = self.data.write().unwrap();
*lock_data = None;
app_action |= AppAction::BackendTask(BackendTask::WithdrawalTask(
WithdrawalsTask::QueryWithdrawals(
self.filter_status_mix.clone(),
self.pagination_items_per_page.into(),
None,
true,
true,
)
));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Refactor repeated code in filter status checkboxes

Each checkbox change handler repeats the same code for updating filters and requesting data. Consider refactoring this repetitive code into a separate method or closure to adhere to the DRY principle.

You can create a helper function to handle the common logic:

fn handle_filter_change(&mut self) {
    self.util_build_combined_filter_status_mix();
    self.requested_data = true;
    let mut lock_data = self.data.write().unwrap();
    *lock_data = None;
    app_action |= AppAction::BackendTask(BackendTask::WithdrawalTask(
        WithdrawalsTask::QueryWithdrawals(
            self.filter_status_mix.clone(),
            self.pagination_items_per_page.into(),
            None,
            true,
            true,
        ),
    ));
}

Then, replace each checkbox change block with a call to this function:

if ui.checkbox(&mut self.filter_status_queued, "Queued").changed() {
    self.handle_filter_change();
}

This refactoring reduces code duplication and improves maintainability.

Comment on lines +474 to +477
fn display_message(&mut self, message: &str, message_type: MessageType) {
self.error_message = Some(message.to_string());
self.requested_data = false;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Unused parameter 'message_type' in 'display_message' method

The message_type parameter is not used within the display_message method. Consider removing it if it's unnecessary, or implement logic based on message_type.

Apply this diff to remove the unused parameter:

-fn display_message(&mut self, message: &str, message_type: MessageType) {
+fn display_message(&mut self, message: &str) {
    self.error_message = Some(message.to_string());
    self.requested_data = false;
}

If message_type is intended for future use, consider implementing the relevant logic.

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +163 to 165
SortColumn::AwardedTo => a.awarded_to.cmp(&b.awarded_to),
};

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Handle Comparison of Optional Fields in Sorting

In the sort_contested_names function, when sorting by SortColumn::AwardedTo, you directly compare Option<Identifier> values using a.awarded_to.cmp(&b.awarded_to). This may not provide the intended sorting behavior since Option types are compared based on their Some and None variants.

Consider unwrapping the Option values with a default or defining a custom comparison to ensure consistent and meaningful sorting. For example:

-    SortColumn::AwardedTo => a.awarded_to.cmp(&b.awarded_to),
+    SortColumn::AwardedTo => {
+        let a_id = a.awarded_to.as_ref().map(|id| id.to_string(Encoding::Base58)).unwrap_or_default();
+        let b_id = b.awarded_to.as_ref().map(|id| id.to_string(Encoding::Base58)).unwrap_or_default();
+        a_id.cmp(&b_id)
+    },
📝 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.

Suggested change
SortColumn::AwardedTo => a.awarded_to.cmp(&b.awarded_to),
};
SortColumn::AwardedTo => {
let a_id = a.awarded_to.as_ref().map(|id| id.to_string(Encoding::Base58)).unwrap_or_default();
let b_id = b.awarded_to.as_ref().map(|id| id.to_string(Encoding::Base58)).unwrap_or_default();
a_id.cmp(&b_id)
},
};

Comment on lines +863 to +868
if self.dpns_subscreen == DPNSSubscreen::Owned && !self.local_dpns_names.is_empty()
{
self.render_table_local_dpns_names(ui);
}
// Render the "no active contests" message if none exist
self.render_no_active_contests(ui);
self.render_no_active_contests_or_owned_names(ui);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Prevent Unnecessary Display of 'No Active Contests' Message

When rendering the UI, if the subscreen is DPNSSubscreen::Owned and local_dpns_names is not empty, both render_table_local_dpns_names(ui) and render_no_active_contests_or_owned_names(ui) are called. This results in the "No owned usernames." message being displayed even when owned names are present.

To fix this logical error, add an else clause to prevent the unnecessary message display:

     if has_contested_names {
         // Existing rendering logic
-    } else if self.dpns_subscreen == DPNSSubscreen::Owned && !self.local_dpns_names.is_empty()
-    {
+    } else if self.dpns_subscreen == DPNSSubscreen::Owned && !self.local_dpns_names.is_empty() {
         self.render_table_local_dpns_names(ui);
     } else {
         // Render the "no active contests" message if none exist
         self.render_no_active_contests_or_owned_names(ui);
     }
📝 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.

Suggested change
if self.dpns_subscreen == DPNSSubscreen::Owned && !self.local_dpns_names.is_empty()
{
self.render_table_local_dpns_names(ui);
}
// Render the "no active contests" message if none exist
self.render_no_active_contests(ui);
self.render_no_active_contests_or_owned_names(ui);
if has_contested_names {
// Existing rendering logic
} else if self.dpns_subscreen == DPNSSubscreen::Owned && !self.local_dpns_names.is_empty() {
self.render_table_local_dpns_names(ui);
} else {
// Render the "no active contests" message if none exist
self.render_no_active_contests_or_owned_names(ui);
}

Comment on lines +619 to +624
let datetime = DateTime::from_timestamp(
dpns_info.acquired_at as i64 / 1000,
((dpns_info.acquired_at % 1000) * 1_000_000) as u32,
)
.map(|dt| dt.to_string())
.unwrap_or_else(|| "Invalid timestamp".to_string());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix Incorrect Timestamp Conversion Method

The use of DateTime::from_timestamp is incorrect because DateTime does not have a from_timestamp method. Instead, you should use Utc.timestamp_millis_opt() to convert a timestamp in milliseconds to a DateTime<Utc>. Additionally, chaining .map() suggests that from_timestamp returns an Option or Result, which is not accurate.

Apply this diff to correct the timestamp conversion:

-    let datetime = DateTime::from_timestamp(
-        dpns_info.acquired_at as i64 / 1000,
-        ((dpns_info.acquired_at % 1000) * 1_000_000) as u32,
-    )
-    .map(|dt| dt.to_string())
-    .unwrap_or_else(|| "Invalid timestamp".to_string());
-
-    row.col(|ui| {
-        ui.label(datetime);
-    });
+    if let LocalResult::Single(datetime) = Utc.timestamp_millis_opt(dpns_info.acquired_at as i64) {
+        let datetime_str = datetime.to_string();
+        row.col(|ui| {
+            ui.label(datetime_str);
+        });
+    } else {
+        row.col(|ui| {
+            ui.label("Invalid timestamp");
+        });
+    }

Committable suggestion skipped: line range outside the PR's diff.

@ogabrielides ogabrielides marked this pull request as draft November 1, 2024 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants