feat: view more keys pop-up in identities screen #25
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 pull request introduces significant modifications across multiple files, primarily enhancing the management of withdrawal statuses and contested names within the application. Key changes include the addition of new screens and subscreens, updates to 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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (12)
src/ui/components/dpns_subscreen_chooser_panel.rs (1)
8-12: Remove unnecessary catch-all pattern
Since the subscreens vector only contains known variants (Active, Past, Owned), the catch-all _ => {} pattern in the match expression is unnecessary and can be removed.
- _ => {}Also applies to: 47-47
src/context.rs (1)
60-63: Consider adjusting indentation for consistency
The indentation of these lines differs from the surrounding code style. Consider aligning with the dpns_contract initialization above.
- 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(),
+ )
+ .expect("expected to get withdrawal contract");src/ui/mod.rs (2)
Line range hint 164-166: Improve error handling for NetworkChooser
Using unreachable!() for NetworkChooser could lead to runtime panics. Consider a more graceful error handling approach.
Consider this alternative:
- ScreenType::NetworkChooser => {
- unreachable!()
- }
+ ScreenType::NetworkChooser => {
+ Screen::NetworkChooserScreen(NetworkChooserScreen::new(app_context))
+ }Line range hint 292-292: Fix incorrect screen type mapping for AddNewIdentityScreen
The AddNewIdentityScreen is incorrectly mapped to ScreenType::AddExistingIdentity.
Apply this fix:
- Screen::AddNewIdentityScreen(_) => ScreenType::AddExistingIdentity,
+ Screen::AddNewIdentityScreen(_) => ScreenType::AddNewIdentity,src/app.rs (2)
120-125: Consider using a factory pattern for screen initialization.
While the current implementation is functional, creating three separate instances of DPNSContestedNamesScreen with different subscreens might lead to code duplication. Consider introducing a factory pattern to centralize screen creation logic and improve maintainability.
Example approach:
struct ScreenFactory;
impl ScreenFactory {
fn create_dpns_screen(context: &Arc<AppContext>, subscreen: DPNSSubscreen) -> DPNSContestedNamesScreen {
DPNSContestedNamesScreen::new(context, subscreen)
}
}Line range hint 172-194: Consider using const or static definitions for screen mappings.
While the current implementation works, the screen type to screen instance mapping could benefit from stronger type safety and compile-time verification.
Consider using a const or static definition:
use lazy_static::lazy_static;
lazy_static! {
static ref SCREEN_MAPPINGS: [(RootScreenType, fn(&Arc<AppContext>) -> Screen); 7] = [
(RootScreenType::RootScreenDPNSActiveContests, |ctx| Screen::DPNSContestedNamesScreen(
DPNSContestedNamesScreen::new(ctx, DPNSSubscreen::Active)
)),
// ... other mappings
];
}src/platform/withdrawals/mod.rs (1)
105-112: Simplify error handling when retrieving sum tree element
The error message "could not get sum tree value for current withdrawal maximum" is repeated. Consider reducing repetition to improve code clarity.
You can refactor the code as follows:
-let sum_tree_element_option = elements
- .get(&WITHDRAWAL_TRANSACTIONS_SUM_AMOUNT_TREE_KEY.to_vec())
- .ok_or_else(|| {
- "could not get sum tree value for current withdrawal maximum".to_string()
- })?;
-
-let sum_tree_element = sum_tree_element_option.as_ref().ok_or_else(|| {
- "could not get sum tree value for current withdrawal maximum".to_string()
-})?;
+let sum_tree_element = elements
+ .get(&WITHDRAWAL_TRANSACTIONS_SUM_AMOUNT_TREE_KEY.to_vec())
+ .and_then(|elem_option| elem_option.as_ref())
+ .ok_or_else(|| "could not get sum tree value for current withdrawal maximum".to_string())?;src/ui/withdraws_status_screen.rs (1)
218-218: Remove debugging println! statement
The println! statement on line 218 appears to be for debugging purposes and should be removed or replaced with proper logging if needed.
Apply this diff to remove the println!:
- println!("computing with:{}", self.pagination_items_per_page.get() as usize);src/ui/identities/identities_screen.rs (2)
395-408: Indicate key ownership in the "View More" pop-up
In the "View More" pop-up, keys from the main identity and the voter identity are displayed together without differentiation. To enhance clarity, consider adding labels or headers to indicate which keys belong to the main identity and which belong to the voter identity.
You can modify the code as follows:
// Before displaying main identity keys
+ui.label("Main Identity Keys:");
for (key_id, key) in main_identity_rest_keys {
// Display keys...
}
// Before displaying voter identity keys
if let Some((voter_identity, _)) = qualified_identity.associated_voter_identity.as_ref() {
+ ui.label("Voter Identity Keys:");
for (key_id, key) in voter_public_keys_vec.iter() {
// Display keys...
}
}Line range hint 90-96: Handle all possible variants of the Purpose enum
The match statement on key.purpose() covers specific variants of the Purpose enum. To prevent potential runtime errors if new variants are added in the future, consider adding a wildcard arm to handle any unexpected variants.
Modify the match statement:
match key.purpose() {
Purpose::AUTHENTICATION => format!("A{}", key.id()),
Purpose::ENCRYPTION => format!("En{}", key.id()),
Purpose::DECRYPTION => format!("De{}", key.id()),
Purpose::TRANSFER => format!("T{}", key.id()),
Purpose::SYSTEM => format!("S{}", key.id()),
Purpose::VOTING => format!("V{}", key.id()),
Purpose::OWNER => format!("O{}", key.id()),
+ _ => format!("Unknown{}", key.id()), // Handle any other variants
}src/ui/dpns_contested_names_screen.rs (2)
206-231: Update the no-data message for the Past Contests subscreen
In the render_no_active_contests function, the message for DPNSSubscreen::Past reads:
"No active or past contests at the moment."This could be confusing for users viewing the Past Contests screen. Consider updating the message to:
"No past contests at the moment."417-543: Add unit tests for the new render_table_past_contests function
The render_table_past_contests function introduces significant new functionality for displaying past contests. To ensure reliability and prevent regressions, consider adding unit tests that cover various scenarios, such as different contest states and data edge cases.
🛑 Comments failed to post (14)
src/ui/components/dpns_subscreen_chooser_panel.rs (1)
27-49: 🛠️ Refactor suggestion
Consider extracting navigation logic
The navigation logic could be made more maintainable by extracting it into a separate function. This would improve readability and make it easier to modify the navigation behavior in the future.
+ fn get_screen_type(subscreen: DPNSSubscreen) -> RootScreenType { + match subscreen { + DPNSSubscreen::Active => RootScreenType::RootScreenDPNSActiveContests, + DPNSSubscreen::Past => RootScreenType::RootScreenDPNSPastContests, + DPNSSubscreen::Owned => RootScreenType::RootScreenDPNSOwnedNames, + } + } + for subscreen in subscreens { if ui.button(subscreen.display_name()).clicked() { - match subscreen { - DPNSSubscreen::Active => { - action = AppAction::SetMainScreen( - RootScreenType::RootScreenDPNSActiveContests, - ) - } - DPNSSubscreen::Past => { - action = AppAction::SetMainScreen( - RootScreenType::RootScreenDPNSPastContests, - ) - } - DPNSSubscreen::Owned => { - action = AppAction::SetMainScreen( - RootScreenType::RootScreenDPNSOwnedNames, - ) - } - _ => {} - } + action = AppAction::SetMainScreen(get_screen_type(subscreen)); }📝 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.fn get_screen_type(subscreen: DPNSSubscreen) -> RootScreenType { match subscreen { DPNSSubscreen::Active => RootScreenType::RootScreenDPNSActiveContests, DPNSSubscreen::Past => RootScreenType::RootScreenDPNSPastContests, DPNSSubscreen::Owned => RootScreenType::RootScreenDPNSOwnedNames, } } for subscreen in subscreens { // Show the subscreen name as a clickable option if ui.button(subscreen.display_name()).clicked() { action = AppAction::SetMainScreen(get_screen_type(subscreen)); }src/context.rs (1)
140-166: 🛠️ Refactor suggestion
Consider improving error handling and performance
The implementation has several areas that could be enhanced:
Error Handling:
- Currently swallows errors and returns empty vectors
- Consider propagating errors or providing more detailed logging
Performance:
- Loads all contested names and identities into memory
- Could be inefficient with large datasets
- Consider implementing pagination or streaming
Code Structure:
- Method could be split into smaller functions for better readability
Consider this refactoring:
- pub fn owned_contested_names(&self) -> Result<Vec<ContestedName>> { - let all_contested_names = self.all_contested_names().unwrap_or_else(|e| { - tracing::error!("Failed to load contested names: {:?}", e); - Vec::new() // Use default value if loading fails - }); - let local_qualified_identities = - self.load_local_qualified_identities().unwrap_or_else(|e| { - tracing::error!("Failed to load local qualified identities: {:?}", e); - Vec::new() // Use default value if loading fails - }); - - let owned_contested_names = all_contested_names - .into_iter() - .filter(|contested_name| { - contested_name - .awarded_to - .as_ref() - .map_or(false, |awarded_to| { - local_qualified_identities - .iter() - .any(|identity| identity.identity.id() == awarded_to) - }) - }) - .collect(); - - Ok(owned_contested_names) + pub fn owned_contested_names(&self) -> Result<Vec<ContestedName>> { + let all_contested_names = self.all_contested_names() + .map_err(|e| { + tracing::error!("Failed to load contested names: {e:?}"); + e + })?; + + let local_qualified_identities = self.load_local_qualified_identities() + .map_err(|e| { + tracing::error!("Failed to load local qualified identities: {e:?}"); + e + })?; + + let local_identity_ids: std::collections::HashSet<_> = local_qualified_identities + .iter() + .map(|identity| identity.identity.id()) + .collect(); + + Ok(all_contested_names + .into_iter() + .filter(|contested_name| { + contested_name + .awarded_to + .as_ref() + .map_or(false, |awarded_to| local_identity_ids.contains(awarded_to)) + }) + .collect()) }This refactoring:
- Properly propagates errors while maintaining logging
- Uses a HashSet for more efficient identity lookups
- Improves code readability with better structure
📝 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.pub fn owned_contested_names(&self) -> Result<Vec<ContestedName>> { let all_contested_names = self.all_contested_names() .map_err(|e| { tracing::error!("Failed to load contested names: {e:?}"); e })?; let local_qualified_identities = self.load_local_qualified_identities() .map_err(|e| { tracing::error!("Failed to load local qualified identities: {e:?}"); e })?; let local_identity_ids: std::collections::HashSet<_> = local_qualified_identities .iter() .map(|identity| identity.identity.id()) .collect(); Ok(all_contested_names .into_iter() .filter(|contested_name| { contested_name .awarded_to .as_ref() .map_or(false, |awarded_to| local_identity_ids.contains(awarded_to)) }) .collect()) }src/ui/mod.rs (1)
58-81: 🛠️ Refactor suggestion
Consider improving enum integer mapping maintainability
The current implementation uses hardcoded sequential integers for screen type mapping. This approach is fragile and requires manual updates when adding or removing variants.
Consider one of these alternatives:
- Use a derive macro like
num_derive::FromPrimitiveandnum_derive::ToPrimitive:use num_derive::{FromPrimitive, ToPrimitive}; #[derive(FromPrimitive, ToPrimitive)] pub enum RootScreenType { RootScreenIdentities, RootScreenDPNSActiveContests, // ... other variants }
- Use const values for better documentation and maintenance:
impl RootScreenType { const IDENTITIES: u32 = 0; const DPNS_ACTIVE_CONTESTS: u32 = 1; // ... other constants pub fn to_int(self) -> u32 { match self { Self::RootScreenIdentities => Self::IDENTITIES, Self::RootScreenDPNSActiveContests => Self::DPNS_ACTIVE_CONTESTS, // ... other matches } } }src/app.rs (1)
146-154: 🛠️ Refactor suggestion
Consider extracting network-specific screen initialization logic.
The current implementation duplicates screen initialization code for different networks. This could be refactored to reduce duplication and improve maintainability.
Consider creating a helper method:
impl AppState { fn initialize_network_screens(context: &Arc<AppContext>) -> ( IdentitiesScreen, DPNSContestedNamesScreen, DPNSContestedNamesScreen, DPNSContestedNamesScreen, TransitionVisualizerScreen, DocumentQueryScreen, WithdrawsStatusScreen, ) { ( IdentitiesScreen::new(context), DPNSContestedNamesScreen::new(context, DPNSSubscreen::Active), DPNSContestedNamesScreen::new(context, DPNSSubscreen::Past), DPNSContestedNamesScreen::new(context, DPNSSubscreen::Owned), TransitionVisualizerScreen::new(context), DocumentQueryScreen::new(context), WithdrawsStatusScreen::new(context), ) } }src/platform/withdrawals/mod.rs (3)
148-150: 🛠️ Refactor suggestion
Avoid hardcoding
PlatformVersion::latest()Using
PlatformVersion::latest()may lead to unexpected behavior if the platform version updates. Consider retrieving the platform version from the SDK or configuration to ensure consistent behavior across different environments.
63-131: 🛠️ Refactor suggestion
Implement pagination for withdrawal queries
The
query_withdrawalsmethod sets a limit of 100 on the number of documents retrieved but does not handle pagination. If there are more than 100 withdrawal documents, some records may be missed. Consider implementing pagination to retrieve all relevant documents.
201-202:
⚠️ Potential issueCorrect error handling when converting script to address
The method
Address::from_scriptreturns anOption<Address>, not aResult. Usingmap_erron anOptionis incorrect. Replacemap_errwithok_or_elseto properly handle the potentialNonevalue.Apply this diff to fix the error handling:
-let address = Address::from_script(&output_script, network) - .map_err(|_| "expected a valid address".to_string())?; +let address = Address::from_script(&output_script, network) + .ok_or_else(|| "expected a valid address".to_string())?;📝 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.let address = Address::from_script(&output_script, network) .ok_or_else(|| "expected a valid address".to_string())?;src/ui/withdraws_status_screen.rs (3)
219-220:
⚠️ Potential issuePotential underflow in pagination calculation when there are no withdrawals
When
data.withdrawals.len()is 0,total_pagesbecomes 0, andtotal_pages - 1underflows, leading to an incorrect current page index.Apply this diff to handle the case when there are no withdrawals:
let total_pages = (data.withdrawals.len() + (self.pagination_items_per_page.get() as usize) - 1) / (self.pagination_items_per_page.get() as usize); +let total_pages = if total_pages == 0 { 1 } else { total_pages }; let mut current_page = self.pagination_current_page.get().min(total_pages - 1); // Clamp to valid page range📝 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.let total_pages = (data.withdrawals.len() + (self.pagination_items_per_page.get() as usize) - 1) / (self.pagination_items_per_page.get() as usize); let total_pages = if total_pages == 0 { 1 } else { total_pages }; let mut current_page = self.pagination_current_page.get().min(total_pages - 1); // Clamp to valid page range
65-71:
⚠️ Potential issueInconsistency in initializing
filter_status_mixwithEXPIREDstatusAlthough
filter_status_expiredis initialized tofalse, theEXPIREDstatus is included infilter_status_mix. This may lead to unexpected behavior in filtering.Apply this diff to correct the inconsistency:
filter_status_mix: RefCell::new(vec![ Value::U8(WithdrawalStatus::QUEUED as u8), Value::U8(WithdrawalStatus::POOLED as u8), Value::U8(WithdrawalStatus::BROADCASTED as u8), Value::U8(WithdrawalStatus::COMPLETE as u8), - Value::U8(WithdrawalStatus::EXPIRED as u8), ]),📝 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.filter_status_mix: RefCell::new(vec![ Value::U8(WithdrawalStatus::QUEUED as u8), Value::U8(WithdrawalStatus::POOLED as u8), Value::U8(WithdrawalStatus::BROADCASTED as u8), Value::U8(WithdrawalStatus::COMPLETE as u8), ]),
235-258: 🛠️ Refactor suggestion
Indicate current sort column in table headers
Currently, the table headers do not visually indicate which column is being used for sorting. Consider updating
selectable_labelto reflect the current sort column.Apply this diff to reflect the current sort column in the table headers:
header.col(|ui| { + let is_selected = self.sort_column.get() == Some(SortColumn::DateTime); - if ui.selectable_label(false, "Date / Time").clicked() { + if ui.selectable_label(is_selected, "Date / Time").clicked() { self.handle_column_click(SortColumn::DateTime); } });Repeat similar changes for the other columns to enhance user experience.
Committable suggestion was skipped due to low confidence.
src/ui/identities/identities_screen.rs (3)
48-55: 🛠️ Refactor suggestion
Refactor duplicate code for displaying truncated identity IDs
The logic for truncating the identity ID to the first 8 characters is duplicated in
show_identity_idandshow_more_keys. Consider refactoring this into a helper function to adhere to the DRY (Don't Repeat Yourself) principle.You can create a helper function like:
fn truncated_identity_id(identity: &Identity, encoding: Encoding) -> String { identity .id() .to_string(encoding) .chars() .take(8) .collect::<String>() }And use it in both places:
// In show_identity_id -let identifier_as_string_first_8_chars: String = qualified_identity - .identity - .id() - .to_string(encoding) - .chars() - .take(8) - .collect(); +let identifier_as_string_first_8_chars = truncated_identity_id(&qualified_identity.identity, encoding);// In show_more_keys -ui.label(format!( - "{}...", - identity - .id() - .to_string(Encoding::Base58) - .chars() - .take(8) - .collect::<String>() -)); +ui.label(format!("{}...", truncated_identity_id(identity, Encoding::Base58)));
377-385: 🛠️ Refactor suggestion
Refactor duplicate code for displaying truncated identity IDs
As mentioned earlier, the code for truncating the identity ID is duplicated here. By using the suggested helper function, you can eliminate this duplication.
Apply the helper function
truncated_identity_idas shown in the previous comment.
411-413: 🛠️ Refactor suggestion
Simplify pop-up closing logic by removing unnecessary flag
Instead of using the
close_more_keys_popupflag to close the pop-up, you can directly setself.show_more_keys_popup = Nonewhen the "Close" button is clicked. This simplifies the code and removes the need for an additional boolean flag.Apply this diff to simplify the closing logic:
// In show_more_keys function if ui.button("Close").clicked() { - self.close_more_keys_popup = true; + self.show_more_keys_popup = None; }Then, you can remove the related code in the
uimethod:// In ui method // Remove the check for close_more_keys_popup -// Close the pop-up if the flag is set -if self.close_more_keys_popup { - self.show_more_keys_popup = None; - self.close_more_keys_popup = false; -}And remove the
close_more_keys_popupfield from the struct:// In IdentitiesScreen struct pub struct IdentitiesScreen { // ... - close_more_keys_popup: bool, // Remove this field }📝 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 ui.button("Close").clicked() { self.show_more_keys_popup = None; }src/ui/dpns_contested_names_screen.rs (1)
758-768:
⚠️ Potential issueEnsure correct rendering for the 'Owned' subscreen
In the
uifunction, when handlingDPNSSubscreen::Owned, the code callsself.render_table_past_contests(ui);. Since this function is tailored for past contests, it may not display owned usernames appropriately.Consider creating a dedicated function, such as
render_table_owned_names, to render the owned usernames with relevant columns and data specific to the user's identities. This will provide a clearer and more accurate representation of owned usernames.
…h-evo-tool into feat/identity-add-keys-popup
Display only the first 3 keys for an identity and if they have more than 3 keys, display a button to view more keys, which opens a pop-up with the rest of the keys.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation