feat: register usernames#3
Conversation
…mac_os" This reverts commit 15cb91e.
Feat/register usernames2
WalkthroughThe changes introduced in this pull request enhance the functionality of the identity management system by adding new methods, structs, and modules related to registering a DPNS name. Key modifications include the addition of the Changes
Poem
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: 12
🧹 Outside diff range and nitpick comments (5)
src/model/qualified_identity.rs (1)
181-191: LGTM: Newdocument_signing_keymethod enhances functionality.The new
document_signing_keymethod is a valuable addition to theQualifiedIdentitystruct. It effectively retrieves an appropriate public key for document signing based on the provided document type's security level requirement. This approach enhances the flexibility and security of the identity management system.Consider adding a log message or debug assertion when no matching key is found to aid in troubleshooting:
pub fn document_signing_key( &self, document_type: &DocumentTypeRef, ) -> Option<&IdentityPublicKey> { let key = self.identity.get_first_public_key_matching( Purpose::AUTHENTICATION, HashSet::from([document_type.security_level_requirement()]), HashSet::from(KeyType::all_key_types()), false, ); if key.is_none() { log::debug!("No matching key found for document type: {:?}", document_type); } key }This addition would help developers identify issues related to missing or incompatible keys during development and debugging.
src/platform/identity/mod.rs (1)
39-43: LGTM: New struct for DPNS name registration input.The
RegisterDpnsNameInputstruct is well-designed and includes the necessary fields for DPNS name registration. The public visibility and derive macros are appropriate.Consider adding a doc comment to explain the purpose of this struct and its fields.
You could add a doc comment like this:
/// Input data for registering a DPNS name. /// /// # Fields /// * `qualified_identity` - The qualified identity associated with the DPNS name. /// * `name_input` - The desired DPNS name to be registered. pub struct RegisterDpnsNameInput { // ... existing fields ... }src/ui/identities/identities_screen.rs (1)
Line range hint
265-267: Consider implementing the "Transfer" functionalityThe "Transfer" button is currently a placeholder without any implemented functionality. To complete the feature set and enhance user experience, consider implementing the transfer functionality for identities.
Would you like assistance in designing and implementing the transfer functionality?
src/ui/dpns_contested_names_screen.rs (1)
177-188: LGTM: New "Register Name" button addedThe new "Register Name" button is correctly implemented in the top panel, aligning with the PR objective of adding functionality to register usernames. The implementation is consistent with the existing UI structure and uses appropriate action types.
Consider extracting the button definitions into a separate constant or method to improve readability. For example:
fn get_top_panel_buttons() -> Vec<(&'static str, DesiredAppAction)> { vec![ ( "Register Name", DesiredAppAction::AddScreenType(ScreenType::RegisterDpnsName), ), ( "Refresh", DesiredAppAction::BackendTask(BackendTask::ContestedResourceTask( ContestedResourceTask::QueryDPNSContestedResources, )), ), ] }Then in the
uimethod:let action = add_top_panel( ctx, &self.app_context, vec![("Dash Evo Tool", AppAction::None)], get_top_panel_buttons(), );This would make the
uimethod cleaner and easier to maintain.src/ui/identities/register_dpns_name_screen.rs (1)
63-79: Sort identities for improved user experienceIn the identity selection ComboBox, identities are listed in the order they are loaded, which may not be intuitive for users.
Consider sorting the
qualified_identitiesalphabetically by alias or identity ID. This will make it easier for users to find and select the desired identity.Apply this diff to sort the identities:
impl RegisterDpnsNameScreen { pub fn new(app_context: &Arc<AppContext>) -> Self { - let qualified_identities = app_context + let mut qualified_identities = app_context .load_local_qualified_identities() .unwrap_or_default(); + qualified_identities.sort_by(|a, b| a.alias.cmp(&b.alias)); let selected_qualified_identity = qualified_identities.first().cloned(); Self {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (9)
mac_os/AppIcons/Assets.xcassets/AppIcon.appiconset/1024.pngis excluded by!**/*.pngmac_os/AppIcons/Assets.xcassets/AppIcon.appiconset/128.pngis excluded by!**/*.pngmac_os/AppIcons/Assets.xcassets/AppIcon.appiconset/16.pngis excluded by!**/*.pngmac_os/AppIcons/Assets.xcassets/AppIcon.appiconset/256.pngis excluded by!**/*.pngmac_os/AppIcons/Assets.xcassets/AppIcon.appiconset/32.pngis excluded by!**/*.pngmac_os/AppIcons/Assets.xcassets/AppIcon.appiconset/512.pngis excluded by!**/*.pngmac_os/AppIcons/Assets.xcassets/AppIcon.appiconset/64.pngis excluded by!**/*.pngmac_os/AppIcons/appstore.pngis excluded by!**/*.pngmac_os/AppIcons/playstore.pngis excluded by!**/*.png
📒 Files selected for processing (10)
- src/model/qualified_identity.rs (3 hunks)
- src/platform/identity/load_identity.rs (1 hunks)
- src/platform/identity/mod.rs (3 hunks)
- src/platform/identity/register_dpns_name.rs (1 hunks)
- src/ui/dpns_contested_names_screen.rs (2 hunks)
- src/ui/identities/add_existing_identity_screen.rs (4 hunks)
- src/ui/identities/identities_screen.rs (1 hunks)
- src/ui/identities/mod.rs (1 hunks)
- src/ui/identities/register_dpns_name_screen.rs (1 hunks)
- src/ui/mod.rs (7 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/ui/identities/mod.rs
🧰 Additional context used
🔇 Additional comments (27)
src/platform/identity/load_identity.rs (1)
29-29: LGTM: Simplified handling ofkeys_input.The change to ignore
keys_inputusing the underscore pattern simplifies the code by explicitly showing that this field is not used in the function. This aligns with the removal of unused variables and improves code clarity.To ensure that
keys_inputis indeed no longer needed, please run the following script:If these searches return no results (other than the line we just changed), it confirms that
keys_inputis no longer used and can be safely ignored.✅ Verification successful
: The usage of
keys_inputhas been successfully removed fromsrc/platform/identity/load_identity.rs. The change to ignorekeys_inputdoes not impact any other part of the function or file, ensuring code clarity and safety.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that keys_input is not used elsewhere in the function or file # Test 1: Check for any remaining usage of keys_input in the file rg --type rust 'keys_input' src/platform/identity/load_identity.rs # Test 2: Check for any comments mentioning keys_input that might need updating rg --type rust -i 'keys.?input' src/platform/identity/load_identity.rsLength of output: 193
src/model/qualified_identity.rs (3)
6-7: LGTM: New imports for document type handling.The added imports for
DocumentTypeV0GettersandDocumentTypeRefare appropriate for the newdocument_signing_keymethod. They provide the necessary types for working with document types in the identity management system.
181-191: LGTM: Improved specificity incan_sign_with_master_keymethod.The addition of the
IdentityType::Usercheck enhances the method's precision by ensuring that only user identities are considered for signing with the master key. This change aligns well with the expected behavior for different identity types and improves the overall robustness of the code.
Line range hint
1-191: Overall assessment: Changes enhance identity management functionality.The modifications to
src/model/qualified_identity.rssuccessfully implement the new document signing functionality while maintaining good code quality. The changes align well with the PR objectives and enhance the identity management system's capabilities. The newdocument_signing_keymethod and the improvements to thecan_sign_with_master_keymethod contribute to a more robust and flexible system for handling different identity types and document signing requirements.src/platform/identity/mod.rs (4)
3-3: LGTM: New module for DPNS name registration added.The addition of the
register_dpns_namemodule aligns well with the PR objectives and follows proper Rust naming conventions.
51-51: LGTM: New enum variant for DPNS name registration task.The addition of the
RegisterDpnsNamevariant to theIdentityTaskenum is appropriate and aligns well with the PR objectives. It correctly uses the newRegisterDpnsNameInputstruct as its associated data.
244-244: LGTM: New match arm for DPNS name registration task.The addition of the
RegisterDpnsNamematch arm in therun_identity_taskmethod is correct and consistent with the existing pattern. It properly handles the new task variant.However, please ensure that the
register_dpns_namemethod is implemented in theAppContextstruct, as it's not visible in the current file.Run the following script to verify the
register_dpns_namemethod implementation:✅ Verification successful
To verify the implementation of the
register_dpns_namemethod, I need to examine its contents. I'll use a shell script to view the file.
The
register_dpns_namemethod is indeed implemented in theAppContextstruct, as expected. The implementation is located in the filesrc/platform/identity/register_dpns_name.rs.The method appears to be correctly implemented, following the expected pattern for registering a DPNS name:
- It creates a preorder document to reserve the name.
- It then creates the actual domain document.
- Finally, it submits both documents to the platform.
The implementation includes proper error handling, uses necessary cryptographic operations for security, and is consistent with its usage in the
run_identity_taskmethod.Therefore, the review comment's concern about the visibility of this method has been addressed. The method exists and is implemented correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of register_dpns_name method # Test: Search for the register_dpns_name method implementation rg --type rust -e 'fn\s+register_dpns_name' src/Length of output: 530779
Script:
#!/bin/bash cat src/platform/identity/register_dpns_name.rsLength of output: 5615
11-12: Good cleanup of imports, verify completeness.The removal of unused imports and the addition of necessary ones improve code cleanliness. However, it's crucial to ensure all required imports for the new DPNS registration functionality are present.
Please run the following script to verify the imports:
Also applies to: 15-15
✅ Verification successful
To verify the completeness of the imports for the DPNS registration functionality, we need to examine the contents of the 'register_dpns_name.rs' file. Let's generate a shell script to view its contents:
Current imports are sufficient for DPNS registration functionality.
The review of the
register_dpns_name.rsfile shows that while it uses several imports from thedash_sdkcrate, these imports are correctly scoped within that file. Themod.rsfile doesn't need to import all of these, as they are specific to the implementation details of theregister_dpns_namefunction.The current imports in
mod.rs:use dash_sdk::dpp::identity::accessors::IdentityGettersV0; use dash_sdk::dpp::identity::identity_public_key::accessors::v0::IdentityPublicKeyGettersV0;are sufficient for the module-level functionality. They provide the necessary types and traits for working with identities and their public keys, which are essential for the DPNS registration process.
The other required imports for the DPNS registration are correctly placed in the
register_dpns_name.rsfile, keeping the code well-organized and modular.No changes to the imports in
mod.rsare necessary for the DPNS registration functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify imports for DPNS registration functionality # Test: Check for any unused imports rg --type rust '\buse\s+(?!crate|self|super).*?;' src/platform/identity/mod.rs | rg -v '(^|\s)use\s+.*\{.*\}' # Test: Check for any missing imports related to DPNS rg --type rust -e 'DPNS|register_dpns_name' src/platform/identity/Length of output: 530779
Script:
#!/bin/bash # Display the contents of register_dpns_name.rs cat src/platform/identity/register_dpns_name.rsLength of output: 5615
src/ui/identities/add_existing_identity_screen.rs (4)
5-5: LGTM: Import change looks good.The addition of
BackendTaskimport aligns with the changes in the code, particularly in theload_identity_clickedmethod.
Line range hint
183-199: Great job on improving code structure!The removal of the
uiparameter fromload_identity_clickedmethod is a positive change. It separates the UI handling from the business logic, adhering to the principle of separation of concerns. This makes the code more modular and easier to maintain.
290-290: LGTM: Consistent change in method call.This modification aligns perfectly with the earlier change in the
load_identity_clickedmethod signature. It maintains consistency and further reinforces the separation of UI handling from business logic.
Line range hint
1-334: Overall, good improvements with one suggestion for consideration.The changes in this file generally improve the code structure by separating UI handling from business logic. The consistency in these changes is commendable. However, please consider the suggestion about error handling in the
load_testnet_nodes_from_ymlfunction to make the code more robust and maintainable.src/ui/identities/identities_screen.rs (1)
83-83: LGTM: Improved key labeling consistencyThe change from "W{}" to "T{}" for the
Purpose::TRANSFERvariant improves the consistency and intuitiveness of the key labeling system. This aligns well with the naming convention used for other key purposes (e.g., "A" for Authentication, "En" for Encryption) and makes the UI more user-friendly.src/ui/dpns_contested_names_screen.rs (2)
16-16: LGTM: New import for ScreenTypeThe new import
use super::ScreenType;is correctly added and used in theuimethod to support the new "Register Name" functionality.
Line range hint
1-424: Summary: Successfully implemented "Register Name" functionalityThe changes in this file successfully implement the new "Register Name" functionality as described in the PR objectives. The modifications are minimal and well-integrated into the existing code structure. The new button in the top panel provides users with a clear entry point to register a DPNS username, enhancing the application's functionality.
Key points:
- New import for
ScreenTypeadded.- "Register Name" button implemented in the top panel.
- Appropriate action (AddScreenType) used for navigation to the new screen.
The implementation is clean and maintains consistency with the existing codebase. No further changes are required in this file to meet the PR objectives.
src/platform/identity/register_dpns_name.rs (3)
85-87: Ensure input normalization is correctly handled.Using
convert_to_homograph_safe_charshelps prevent homograph attacks, but ensure that all inputs are properly validated and normalized before processing to prevent security vulnerabilities.Consider implementing additional validation checks on
input.name_inputto ensure it meets all required criteria for a DPNS name.
26-31: Ensure cryptographic best practices for entropy and random number generation.Using
StdRngwithfrom_entropyis acceptable, but for cryptographic purposes, consider using a cryptographically secure random number generator likerand::rngs::OsRngto generate entropy and salts.Run the following script to check if
OsRngis available and recommended:#!/bin/bash # Description: Check for usage of cryptographically secure RNGs. # Test: Search for usage of OsRng. Expect: OsRng is available and preferred for cryptographic operations. rg --type rust 'use rand::rngs::OsRng'
56-57: Verify the correctness of the salted domain hash computation.The salted domain is being computed by concatenating the salt with the normalized domain name and appending ".dash". Ensure that this procedure aligns with the DPNS specification for domain name hashing.
Run the following script to cross-reference the hashing implementation:
src/ui/mod.rs (9)
16-16: Importingegui::Contextfor GUI renderingThe import of
egui::Contextis necessary for implementing theuimethod in theScreenLiketrait, which facilitates GUI rendering.
21-21: ImportingRegisterDpnsNameScreenfor new screen functionalityThe addition of
use identities::register_dpns_name_screen::RegisterDpnsNameScreen;correctly brings in the new screen module required for DPNS name registration.
23-23: Importingstd::hash::Hashfor derivingHashtraitImporting
std::hash::Hashis necessary asRootScreenTypenow derives theHashtrait, enabling it to be used in hashed collections or contexts requiring hashing.
100-100: AddingRegisterDpnsNamevariant toScreenTypeenumThe new variant
RegisterDpnsNameis appropriately added to theScreenTypeenum to represent the DPNS name registration screen.
127-129: HandlingRegisterDpnsNameincreate_screenmethodThe
create_screenmethod correctly handles theRegisterDpnsNamevariant by instantiating a newRegisterDpnsNameScreen, ensuring the screen is created when this variant is selected.
162-162: AddingRegisterDpnsNameScreentoScreenenumThe
RegisterDpnsNameScreen(RegisterDpnsNameScreen)variant is added to theScreenenum, allowing the dispatch mechanism to include the new screen type.
183-183: Updatingchange_contextmethod forRegisterDpnsNameScreenThe
change_contextmethod inScreenis updated to handleRegisterDpnsNameScreen, ensuring theapp_contextis correctly set for this screen.
200-201: Using underscore-prefixed parameters for unused variablesBy prefixing parameters with an underscore in
display_messageanddisplay_task_result, the methods explicitly indicate that these parameters are intentionally unused, preventing compiler warnings.
238-238: MappingRegisterDpnsNameScreeninscreen_typemethodThe
screen_typemethod correctly returnsScreenType::RegisterDpnsNamewhenRegisterDpnsNameScreenis active, enabling proper identification of the current screen type.
| fn load_testnet_nodes_from_yml(file_path: &str) -> Option<TestnetNodes> { | ||
| let file_content = fs::read_to_string(file_path).ok()?; | ||
| serde_yaml::from_str(&file_content).ok() | ||
| serde_yaml::from_str(&file_content).expect("expected proper yaml") |
There was a problem hiding this comment.
Consider using ? operator instead of expect.
Using expect here will cause the program to panic if the YAML parsing fails. This might be too strict for production code. Consider using the ? operator to propagate the error instead, allowing for more graceful error handling upstream.
Here's a suggested change:
- serde_yaml::from_str(&file_content).expect("expected proper yaml")
+ serde_yaml::from_str(&file_content).map_err(|e| format!("Failed to parse YAML: {}", e))This change would require updating the function signature to return a Result instead of an Option.
Committable suggestion was skipped due to low confidence.
| pub(super) async fn register_dpns_name( | ||
| &self, | ||
| sdk: &Sdk, | ||
| input: RegisterDpnsNameInput, | ||
| ) -> Result<(), String> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a more descriptive error type instead of String.
Returning Result<(), String> may limit error handling capabilities. Consider defining a custom error type or using a standard error type like anyhow::Error or Box<dyn std::error::Error> to provide more context and improve error handling throughout the application.
| pub(super) async fn register_dpns_name( | ||
| &self, | ||
| sdk: &Sdk, | ||
| input: RegisterDpnsNameInput, | ||
| ) -> Result<(), String> { | ||
| let mut rng = StdRng::from_entropy(); | ||
| let dpns_contract = self.dpns_contract.clone(); | ||
|
|
||
| let qualified_identity = input.qualified_identity; | ||
|
|
||
| let entropy = Bytes32::random_with_rng(&mut rng); | ||
| let preorder_document_type = dpns_contract | ||
| .document_type_for_name("preorder") | ||
| .map_err(|_| "DPNS preorder document type not found".to_string())?; | ||
| let domain_document_type = dpns_contract | ||
| .document_type_for_name("domain") | ||
| .map_err(|_| "DPNS domain document type not found".to_string())?; | ||
|
|
||
| let preorder_id = Document::generate_document_id_v0( | ||
| &dpns_contract.id(), | ||
| &qualified_identity.identity.id(), | ||
| preorder_document_type.name().as_str(), | ||
| entropy.as_slice(), | ||
| ); | ||
| let domain_id = Document::generate_document_id_v0( | ||
| &dpns_contract.id(), | ||
| &qualified_identity.identity.id(), | ||
| domain_document_type.name().as_str(), | ||
| entropy.as_slice(), | ||
| ); | ||
|
|
||
| let salt: [u8; 32] = rng.gen(); | ||
| let mut salted_domain_buffer: Vec<u8> = vec![]; | ||
| salted_domain_buffer.extend(salt); | ||
| salted_domain_buffer | ||
| .extend((convert_to_homograph_safe_chars(&input.name_input) + ".dash").as_bytes()); | ||
| let salted_domain_hash = hash_double(salted_domain_buffer); | ||
|
|
||
| let preorder_document = Document::V0(DocumentV0 { | ||
| id: preorder_id, | ||
| owner_id: qualified_identity.identity.id(), | ||
| properties: BTreeMap::from([( | ||
| "saltedDomainHash".to_string(), | ||
| salted_domain_hash.into(), | ||
| )]), | ||
| revision: None, | ||
| created_at: None, | ||
| updated_at: None, | ||
| transferred_at: None, | ||
| created_at_block_height: None, | ||
| updated_at_block_height: None, | ||
| transferred_at_block_height: None, | ||
| created_at_core_block_height: None, | ||
| updated_at_core_block_height: None, | ||
| transferred_at_core_block_height: None, | ||
| }); | ||
| let domain_document = Document::V0(DocumentV0 { | ||
| id: domain_id, | ||
| owner_id: qualified_identity.identity.id(), | ||
| properties: BTreeMap::from([ | ||
| ("parentDomainName".to_string(), "dash".into()), | ||
| ("normalizedParentDomainName".to_string(), "dash".into()), | ||
| ("label".to_string(), input.name_input.clone().into()), | ||
| ( | ||
| "normalizedLabel".to_string(), | ||
| convert_to_homograph_safe_chars(&input.name_input).into(), | ||
| ), | ||
| ("preorderSalt".to_string(), salt.into()), | ||
| ( | ||
| "records".to_string(), | ||
| BTreeMap::from([( | ||
| "identity".to_string(), | ||
| Into::<dash_sdk::dpp::platform_value::Value>::into( | ||
| qualified_identity.identity.id(), | ||
| ), | ||
| )]) | ||
| .into(), | ||
| ), | ||
| ( | ||
| "subdomainRules".to_string(), | ||
| BTreeMap::from([( | ||
| "allowSubdomains".to_string(), | ||
| Into::<dash_sdk::dpp::platform_value::Value>::into(false), | ||
| )]) | ||
| .into(), | ||
| ), | ||
| ]), | ||
| revision: None, | ||
| created_at: None, | ||
| updated_at: None, | ||
| transferred_at: None, | ||
| created_at_block_height: None, | ||
| updated_at_block_height: None, | ||
| transferred_at_block_height: None, | ||
| created_at_core_block_height: None, | ||
| updated_at_core_block_height: None, | ||
| transferred_at_core_block_height: None, | ||
| }); | ||
|
|
||
| let public_key = qualified_identity | ||
| .document_signing_key(&preorder_document_type) | ||
| .ok_or( | ||
| "Identity doesn't have an authentication key for signing document transitions" | ||
| .to_string(), | ||
| )?; | ||
|
|
||
| let _ = preorder_document | ||
| .put_to_platform_and_wait_for_response( | ||
| sdk, | ||
| preorder_document_type.to_owned_document_type(), | ||
| entropy.0, | ||
| public_key.clone(), | ||
| self.dpns_contract.clone(), | ||
| &qualified_identity, | ||
| ) | ||
| .await; | ||
|
|
||
| let _ = domain_document | ||
| .put_to_platform_and_wait_for_response( | ||
| sdk, | ||
| domain_document_type.to_owned_document_type(), | ||
| entropy.0, | ||
| public_key.clone(), | ||
| self.dpns_contract.clone(), | ||
| &qualified_identity, | ||
| ) | ||
| .await; | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring register_dpns_name into smaller functions for better maintainability.
The register_dpns_name function is lengthy and encompasses multiple responsibilities, such as entropy generation, document creation, and submission. Splitting it into smaller, logically grouped helper functions can improve readability and simplify future maintenance.
| let preorder_document_type = dpns_contract | ||
| .document_type_for_name("preorder") | ||
| .map_err(|_| "DPNS preorder document type not found".to_string())?; | ||
| let domain_document_type = dpns_contract | ||
| .document_type_for_name("domain") | ||
| .map_err(|_| "DPNS domain document type not found".to_string())?; |
There was a problem hiding this comment.
Include underlying errors in map_err for better debugging.
Currently, the original errors are being discarded in the map_err calls. Including the underlying error information can aid in debugging and provide more context.
Apply this diff to include the original errors:
-.map_err(|_| "DPNS preorder document type not found".to_string())?;
+.map_err(|e| format!("DPNS preorder document type not found: {}", e))?;Repeat similarly for the domain document type error handling.
Committable suggestion was skipped due to low confidence.
| let _ = preorder_document | ||
| .put_to_platform_and_wait_for_response( | ||
| sdk, | ||
| preorder_document_type.to_owned_document_type(), | ||
| entropy.0, | ||
| public_key.clone(), | ||
| self.dpns_contract.clone(), | ||
| &qualified_identity, | ||
| ) | ||
| .await; |
There was a problem hiding this comment.
Handle the result of put_to_platform_and_wait_for_response to avoid ignoring errors.
The result of put_to_platform_and_wait_for_response is being discarded with let _. This can lead to unhandled errors if the operation fails. Consider handling the Result to ensure that any errors are appropriately managed.
Apply this diff to handle the result:
-let _ = preorder_document
- .put_to_platform_and_wait_for_response(
- sdk,
- preorder_document_type.to_owned_document_type(),
- entropy.0,
- public_key.clone(),
- self.dpns_contract.clone(),
- &qualified_identity,
- )
- .await;
+preorder_document
+ .put_to_platform_and_wait_for_response(
+ sdk,
+ preorder_document_type.to_owned_document_type(),
+ entropy.0,
+ public_key.clone(),
+ self.dpns_contract.clone(),
+ &qualified_identity,
+ )
+ .await
+ .map_err(|e| format!("Failed to submit preorder document: {}", e))?;📝 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 _ = preorder_document | |
| .put_to_platform_and_wait_for_response( | |
| sdk, | |
| preorder_document_type.to_owned_document_type(), | |
| entropy.0, | |
| public_key.clone(), | |
| self.dpns_contract.clone(), | |
| &qualified_identity, | |
| ) | |
| .await; | |
| preorder_document | |
| .put_to_platform_and_wait_for_response( | |
| sdk, | |
| preorder_document_type.to_owned_document_type(), | |
| entropy.0, | |
| public_key.clone(), | |
| self.dpns_contract.clone(), | |
| &qualified_identity, | |
| ) | |
| .await | |
| .map_err(|e| format!("Failed to submit preorder document: {}", e))?; |
| fn register_dpns_name_clicked(&mut self) -> AppAction { | ||
| let Some(qualified_identity) = self.selected_qualified_identity.as_ref() else { | ||
| return AppAction::None; | ||
| }; |
There was a problem hiding this comment.
Handle missing selected identity with user feedback
In the register_dpns_name_clicked method, if selected_qualified_identity is None, the function returns AppAction::None without any user feedback. This could lead to confusion if the user tries to register a name without selecting an identity.
Consider providing an error message to inform the user to select an identity before proceeding, or disable the "Register Name" button until an identity is selected.
| fn display_message(&mut self, message: &str, message_type: MessageType) { | ||
| if message_type == MessageType::Info && message == "Success" { | ||
| self.register_dpns_name_status = RegisterDpnsNameStatus::Complete; | ||
| } else { | ||
| self.register_dpns_name_status = | ||
| RegisterDpnsNameStatus::ErrorMessage(message.to_string()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid relying on hardcoded success messages
The display_message function checks for a successful registration by matching the message string exactly to "Success". Relying on hardcoded message strings can be fragile and may break if the message changes.
Consider using a dedicated MessageType::Success variant or a status code to determine success more reliably.
| ui.horizontal(|ui| { | ||
| ui.label("Name (without \".dash\"):"); | ||
| ui.text_edit_singleline(&mut self.name_input); | ||
| }); |
There was a problem hiding this comment.
Add input validation for the DPNS name
Currently, there's no validation for name_input before attempting registration. Invalid names could lead to errors during the registration process.
Implement input validation to ensure the DPNS name meets required criteria (e.g., allowed characters, length limitations) before proceeding. This will enhance user experience by providing immediate feedback on invalid input.
| // Set the status to waiting and capture the current time | ||
| let now = SystemTime::now() | ||
| .duration_since(UNIX_EPOCH) | ||
| .expect("Time went backwards") | ||
| .as_secs(); | ||
| self.register_dpns_name_status = RegisterDpnsNameStatus::WaitingForResult(now); | ||
| action = self.register_dpns_name_clicked(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Instant for measuring elapsed time
The code uses SystemTime::now().duration_since(UNIX_EPOCH) to measure elapsed time. This approach can cause errors if the system clock changes or if time moves backwards.
Use std::time::Instant for measuring elapsed time, as it is monotonic and immune to system clock adjustments. Update the RegisterDpnsNameStatus::WaitingForResult variant to store an Instant instead of TimestampMillis.
Apply this diff to implement the change:
- WaitingForResult(TimestampMillis),
+ WaitingForResult(Instant),In the RegisterDpnsNameScreen struct, ensure you import std::time::Instant and adjust the time calculations accordingly.
Committable suggestion was skipped due to low confidence.
| Screen::AddKeyScreen(screen) => ScreenType::AddKeyScreen(screen.identity.clone()), | ||
| Screen::DocumentQueryScreen(_) => ScreenType::DocumentQueryScreen, | ||
| Screen::AddNewIdentityScreen(_) => ScreenType::AddExistingIdentity, | ||
| Screen::RegisterDpnsNameScreen(_) => ScreenType::RegisterDpnsName, |
There was a problem hiding this comment.
Potential mismatch in screen type mapping
In the screen_type method, Screen::AddNewIdentityScreen(_) is mapped to ScreenType::AddExistingIdentity. This seems inconsistent since there's a separate ScreenType::AddNewIdentity. Please verify if this should be ScreenType::AddNewIdentity instead.
1. CRITICAL: Fix BitOrAssign dropping tasks in fetch_unresolved_profiles()
- Changed from action |= AppAction::BackendTask(task) loop (which only
kept the last task) to collecting all tasks into a Vec and dispatching
via AppAction::BackendTasks with Concurrent execution mode.
2. HIGH: Remove dummy [0x02; 33] pubkey in load_payment_history()
- Changed PaymentRecord.to_address from Address to Option<Address>
- Historical records loaded from DB now use None instead of generating
a fake-but-valid P2PKH address from a dummy public key.
3. HIGH: Fix i64 as u64 wrapping for amounts (payments.rs and dashpay.rs)
- Added bounds checking with warning logs for negative amounts,
clamping to 0 instead of silently wrapping.
4. MEDIUM: Eliminate duplicate payment history logic in dashpay.rs
- LoadPaymentHistory task handler now calls the shared
payments::load_payment_history() and converts results, instead of
reimplementing the same logic with different return types.
5. MEDIUM: Fix N+1 database queries in resolve_names_from_local_cache()
- Pre-load all contacts once before the loop and build a HashMap
for O(1) lookups instead of calling load_dashpay_contacts() and
load_dashpay_profile() per request inside the loop.
6. MEDIUM: Fix created_at: None breaking filters in contacts_list.rs
- Contacts from DashPayContactsWithInfo results now get current
timestamp as fallback instead of None, so they work with
'Recent' filter and 'Date added' sort.
7. LOW: Fix 'failed' as failure reason in payment status
- Changed PaymentStatus::Failed to use descriptive 'Transaction failed'
string instead of echoing the literal status column value 'failed'.
8. LOW: Fix i64 as u64 for timestamps in payments.rs
- Added bounds checking with warning for negative timestamps,
consistent with the amount fix.
9. LOW: Both i64 as u64 locations fixed (dashpay.rs duplicate removed
entirely by fix dashpay#4, payments.rs fixed by fixes dashpay#3/dashpay#8).
- Replace assume_checked() with require_network() for address validation (CodeRabbit #2) - Use styled Frame-with-dismiss error display matching Send dialog pattern (CodeRabbit #3) - Don't open dialog when no wallet selected; show MessageBanner instead (CodeRabbit #4) - Extract shared load_bip44_external_addresses() helper to eliminate near-duplicate code between mine and receive dialogs (CodeRabbit #5) - Add backend-side network guard (Regtest/Devnet) for defense-in-depth (CodeRabbit #6) - Rename shadowed ctx binding to refresh_ctx for clarity (CodeRabbit #7) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… mode (#638) * feat(wallet): add Mine Blocks dialog for Regtest/Devnet dev mode Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: add manual test scenarios for mine blocks dialog Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(wallet): close Mine Blocks dialog after Cancel/Mine click The dialog stayed open (in a broken state) after clicking Mine or Cancel because the local `open` variable was written back to `is_open` after the dialog state had already been reset. Pass `is_open` directly to egui's `.open()` and use a separate `close` flag for button-triggered dismissal. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(wallet): address audit findings for Mine Blocks dialog - Wrap `generate_to_address` in `spawn_blocking` to avoid blocking the async runtime thread (HIGH) - Replace `.expect()` on core client lock with `.map_err()?` for graceful error handling instead of panic (HIGH) - Cap block count at 1000 to prevent resource exhaustion on the Core node (HIGH) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(wallet): filter non-numeric input in Mine Blocks block count field Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(wallet): address review comments on Mine Blocks dialog - Replace assume_checked() with require_network() for address validation (CodeRabbit #2) - Use styled Frame-with-dismiss error display matching Send dialog pattern (CodeRabbit #3) - Don't open dialog when no wallet selected; show MessageBanner instead (CodeRabbit #4) - Extract shared load_bip44_external_addresses() helper to eliminate near-duplicate code between mine and receive dialogs (CodeRabbit #5) - Add backend-side network guard (Regtest/Devnet) for defense-in-depth (CodeRabbit #6) - Rename shadowed ctx binding to refresh_ctx for clarity (CodeRabbit #7) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(wallet): address remaining review comments on Mine Blocks dialog - Change MineBlocksSuccess(usize) to MineBlocksSuccess(u64) for type consistency with block_count parameter (Claudius #5) - Align dialog close pattern with Send/Receive: use local `open` variable for egui X button, reset state inside closure for Cancel/Mine buttons (Claudius #6) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Addresses all critical and important findings from the M2 code review. Critical #1: write_core SPV height Changed `WHERE seed_hash = ?2` to `WHERE wallet_id = ?2` in the wallet.last_terminal_block UPDATE. The persister passes wallet_id bytes; the old SQL matched against the seed_hash column which holds different bytes — 0 rows matched, sync height was silently lost on every restart. Critical #2: handle_wallet_unlocked shielded init After register_with_platform_wallet_manager (which may re-key the map), use wallet_id from the Wallet struct for subsequent lookups (initialize_shielded_wallet, queue_shielded_sync) instead of the stale seed_hash variable. Critical #3: WalletDerivationPath stores wrong key Changed qualified_identity_public_key.rs to populate wallet_seed_hash with wallet.wallet_id() instead of wallet.seed_hash(). Post-v40, determine_wallet_info() returns wallet_id bytes, matching the map key. Important #4/#5: wallet selection + UI validation wallets_screen uses wallet_id for persist_selected_wallet_hash and the arc-matches validation check. Finding #6: shielded_wallet_meta in v40 DELETE sweep Added to the cache nuke table list. Wallet.wallet_id is now non-optional (WalletId, not Option<WalletId>). The wallet migration screen (to be implemented) ensures every wallet has wallet_id before the main UI loads. WalletArcRef.seed_hash renamed to wallet_id. No more map_key() fallback — wallet_id is always the canonical key. get_wallets() uses [0u8; 32] as sentinel for NULL wallet_id rows (password wallets pre-migration). The migration screen detects this sentinel and prompts for unlock. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… stale docs Review finding #1 (Critical): Silent data loss on proof encoding write_asset_locks used unwrap_or_default() on bincode encode failure, silently writing an empty blob. Changed to propagate the error via SqlitePersistError::Encode so the flush fails visibly instead of losing the proof. Review finding #4 (Important): Dead code cleanup Deleted store_asset_lock_transaction and update_asset_lock_chain_locked_height from Database — all callers were removed in Item 8.1d. Removed unused imports (Hash, serialize). Review finding #5 (Important): Stale doc comment Updated platform_wallet_bridge.rs module docs to reflect the current state: WalletId = SHA256(root_pub_key || chain_code), both AppContext and PlatformWalletManager keyed consistently. Review finding #2 (FK mismatch) acknowledged as pre-existing: asset_lock_transaction.wallet FK references wallet(seed_hash) but stores wallet_id bytes. FKs are off at runtime. Proper fix deferred to the wallet table PK migration. Review finding #3 (no round-trip test) acknowledged: adding a test for write_asset_locks + load_asset_locks is a follow-up. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Review finding #3: adds test_asset_lock_round_trip which exercises the full write_asset_locks → load path. Creates an AssetLockEntry with Broadcast status, flushes it, then loads and verifies every field: outpoint, transaction, account_index, funding_type, identity_index, amount_duffs, status, proof. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…vation Critical #3: the status derivation in load() used `if proof.is_some()` guard + inner match with `None => unreachable!()`. Replaced with a single flat match using guard patterns — same behavior, no panic risk, cleaner code. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
Add a button in contested resources screen to register a dpns username. It opens a new screen and allows the user to select from the qualified identities which identity to register for and then type in the desired name and broadcast.
Summary by CodeRabbit
Release Notes
New Features
User Interface Enhancements
Bug Fixes
These updates aim to enhance user interaction and streamline the registration process within the application.