feat: choose mn to vote with#8
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes primarily involve updates to the Changes
Possibly related PRs
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: 3
🧹 Outside diff range and nitpick comments (9)
src/platform/contested_names/vote_on_dpns_name.rs (2)
23-25: LGTM: Method signature updated to include voters parameterThe addition of the
voters: Vec<QualifiedIdentity>parameter aligns with the PR objective of allowing users to select specific masternodes for voting. This change enhances the flexibility of the voting process.However, I noticed that the
senderparameter has been renamed to_sender, indicating it's no longer used in the method body. Consider removing this parameter if it's truly unused to keep the method signature clean.Would you like me to propose a modification to remove the unused
_senderparameter?
Line range hint
37-74: LGTM: Method implementation updated to use provided votersThe changes in the method body correctly implement the new functionality of using the provided
votersparameter. The iteration over voters, creation of votes, and submission to the platform are well-structured and align with the PR objective.The error handling for cases where a voter doesn't have an associated voter identity is a good practice and enhances the robustness of the code.
One minor suggestion:
Consider adding a comment explaining the purpose of the hardcoded values for DPNS at lines 37-38. This would improve code readability and maintainability.
+ // Hardcoded values for DPNS: "dash" is the parent domain, and `name` is the subdomain being voted on let index_values = [Value::from("dash"), Value::Text(name.clone())];src/platform/contested_names/mod.rs (1)
36-38: LGTM: Updated match arm for VoteOnDPNSNameThe match arm for
VoteOnDPNSNamehas been correctly updated to handle the newVec<QualifiedIdentity>parameter. This change is consistent with the enum variant update and aligns with the PR objectives.Consider removing the
.to_vec()call ifvotersis already aVec<QualifiedIdentity>:- self.vote_on_dpns_name(name, *vote_choice, voters.to_vec(), sdk, sender) + self.vote_on_dpns_name(name, *vote_choice, voters.clone(), sdk, sender)This avoids creating an unnecessary new vector if
votersis already owned. Ifvotersneeds to be a slice or other iterable type, then the current implementation is correct.src/ui/dpns_contested_names_screen.rs (6)
159-165: Simplify filtering of voting identitiesYou can simplify the filtering of
voting_identitiesby using.filter()instead offilter_map(). This enhances code readability and conciseness.Apply this diff to simplify the filter:
-let voting_identities = local_qualified_identities - .iter() - .filter_map(|id| { - if id.associated_voter_identity.is_some() { - Some(id) - } else { - None - } - }) - .collect_vec(); +let voting_identities = local_qualified_identities + .iter() + .filter(|id| id.associated_voter_identity.is_some()) + .collect_vec();
179-183: Avoid unnecessary cloning when constructing button labelsWhen constructing the button label, cloning
identity.aliasis unnecessary. You can useas_ref()to borrow the value, avoiding the clone.Apply this diff to optimize the code:
let button_label = format!( "{}", identity - .alias - .clone() + .alias + .as_ref() .unwrap_or(identity.identity.id().to_string(Encoding::Base58)) );
185-186: Remove redundant cloning ofidentityOn line 186,
identity.clone().clone()is redundant. A singleclone()is sufficient.Apply this diff to correct the cloning:
if ui.button(button_label).clicked() { // Add the selected identity to the `voters` field - voters.push(identity.clone().clone()); + voters.push(identity.clone());
206-207: Eliminate redundant cloning in 'All' button logicIn the "All" button logic,
identity.clone().clone()is used unnecessarily. Oneclone()call is enough.Apply this diff to streamline the cloning:
for identity in voting_identities.iter() { - voters.push(identity.clone().clone()); + voters.push(identity.clone()); }
169-174: Consider borrowingactioninstead of cloningCloning
actionin theif letpattern may be unnecessary if you can borrow it instead. This can improve performance by avoiding unnecessary copies.Here's how you might modify the code:
-if let ContestedResourceTask::VoteOnDPNSName( - contested_name, - vote_choice, - mut voters, -) = action +if let ContestedResourceTask::VoteOnDPNSName( + ref contested_name, + ref vote_choice, + ref mut voters, +) = &mut actionAdjust the rest of the code within the block accordingly to work with references.
Line range hint
405-417: Maintain consistent messaging in vote confirmation popupsEnsure that the messages in your vote confirmation popups are clear and consistent. This enhances the user experience by providing clear instructions.
Review the messages at lines 405 and 417 to confirm they align with the application's messaging standards.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/platform/contested_names/mod.rs (3 hunks)
- src/platform/contested_names/vote_on_dpns_name.rs (3 hunks)
- src/ui/document_query_screen.rs (1 hunks)
- src/ui/dpns_contested_names_screen.rs (8 hunks)
🧰 Additional context used
🔇 Additional comments (7)
src/platform/contested_names/vote_on_dpns_name.rs (2)
3-3: LGTM: Import statement added for QualifiedIdentityThe addition of the import statement for
QualifiedIdentityis consistent with the changes in the method signature and implementation. This import is necessary for the newvotersparameter.
Line range hint
1-78: LGTM: Successfully implemented voting with selected masternodesThe changes in this file successfully implement the feature of allowing users to select specific masternodes for voting on DPNS usernames. The modifications are well-structured, consistent with the PR objectives, and maintain good code quality. Here's a summary of the key changes:
- Added a new
votersparameter to thevote_on_dpns_namemethod.- Updated the method implementation to iterate over the provided voters.
- Implemented vote creation and submission for each valid voter.
- Added appropriate error handling for invalid voters.
These changes enhance the flexibility of the voting process while maintaining code clarity and robustness.
src/platform/contested_names/mod.rs (3)
8-8: LGTM: Import added for QualifiedIdentityThe import for
QualifiedIdentityis correctly added and is necessary for the updated enum variant.
Line range hint
1-38: Overall: Implementation aligns with PR objectivesThe changes in this file successfully implement the feature to allow users to select specific masternodes or "all" when voting on DPNS usernames. The code is well-structured and consistent with the existing codebase. The main changes include:
- Adding an import for
QualifiedIdentity.- Updating the
VoteOnDPNSNamevariant in theContestedResourceTaskenum.- Modifying the corresponding match arm in the
run_contested_resource_taskmethod.These changes effectively support the new functionality while maintaining code quality. A minor optimization suggestion was made regarding vector creation, but overall, the implementation is sound and meets the PR objectives.
18-18: LGTM: Updated VoteOnDPNSName variant to include multiple votersThe
VoteOnDPNSNamevariant has been correctly updated to include aVec<QualifiedIdentity>, allowing for multiple voters. This change aligns with the PR objectives and enables the selection of specific masternodes or "all" for voting.To ensure consistency across the codebase, please run the following script to check for any locations that might need updates due to this change:
Please review the output and ensure that all occurrences of
VoteOnDPNSNameare updated to include the newVec<QualifiedIdentity>parameter.✅ Verification successful
Verified: All
VoteOnDPNSNameusages correctly includeVec<QualifiedIdentity>The verification confirms that every instance of
VoteOnDPNSNamehas been updated to include theVec<QualifiedIdentity>parameter. No additional changes are required.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usages of VoteOnDPNSName that might need updates # Search for VoteOnDPNSName usage echo "Searching for VoteOnDPNSName usage:" rg --type rust "VoteOnDPNSName\s*\(" -C 3 # Search for pattern matching on VoteOnDPNSName echo "\nSearching for pattern matching on VoteOnDPNSName:" rg --type rust "VoteOnDPNSName\s*\([^)]+\)" -C 3Length of output: 9693
src/ui/dpns_contested_names_screen.rs (2)
308-308: Ensure vote popup is properly displayedAt line 308, you check
if self.show_vote_popup_info.is_some(). Ensure that this condition correctly reflects when the vote popup should be displayed, especially after updates toshow_vote_popup_info.
81-88: Ensure correct contestant ID is used when votingWhen setting
ResourceVoteChoice::TowardsIdentity(contestant.id), confirm thatcontestant.idis the correct identity for voting. This ensures votes are attributed accurately.You can run the following script to verify the contestant IDs:
This script extracts contestant IDs from the code and lists them to help you verify their correctness.
| ContestedResourceTask::VoteOnDPNSName( | ||
| contested_name.normalized_contested_name.clone(), | ||
| ResourceVoteChoice::Abstain, | ||
| vec![], |
There was a problem hiding this comment.
💡 Codebase verification
Masternode selection UI is not implemented.
The addition of vec![] as the third parameter to ContestedResourceTask::VoteOnDPNSName suggests that masternode selection functionality is intended. However, no UI components for selecting masternodes were found in the codebase. This may result in users being unable to select specific masternodes for voting as intended.
- Implement the UI for masternode selection to enable users to choose specific masternodes.
- Ensure that the selection process populates the vector passed to
VoteOnDPNSName.
🔗 Analysis chain
Verify the implementation of masternode selection for voting.
The addition of vec![] as the third parameter to ContestedResourceTask::VoteOnDPNSName aligns with the PR objective of allowing users to select specific masternodes for voting. However, there are a few points to consider:
- The empty vector suggests that no masternodes are selected by default. Is this the intended behavior?
- There's no visible UI element in this file for selecting masternodes. Ensure that the masternode selection is implemented correctly in the relevant UI component.
Consider the following improvements:
- If a default selection is desired, populate the vector with appropriate default values.
- Add a comment explaining the purpose of this parameter for better code readability.
- Verify that the masternode selection UI is properly integrated with this voting action.
To ensure the feature is fully implemented, let's check for the presence of masternode selection UI and its integration:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for UI components related to masternode selection
rg --type rust "fn.*select.*masternode" src
# Check for updates to the ContestedResourceTask enum
rg --type rust "enum ContestedResourceTask" src
# Verify the usage of VoteOnDPNSName with the new parameter
rg --type rust "VoteOnDPNSName.*vec!" src
Length of output: 999
| if let Some((message, action)) = self.show_vote_popup_info.clone() { | ||
| ui.label(message); | ||
|
|
||
| let local_qualified_identities = self | ||
| .app_context | ||
| .db | ||
| .get_local_qualified_identities(&self.app_context) | ||
| .unwrap_or_default(); | ||
|
|
||
| let voting_identities = local_qualified_identities | ||
| .iter() | ||
| .filter_map(|id| { | ||
| if id.associated_voter_identity.is_some() { | ||
| Some(id) | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .collect_vec(); | ||
|
|
||
| ui.horizontal(|ui| { | ||
| if ui.button("Vote Immediate").clicked() { | ||
| app_action = AppAction::BackendTask(BackendTask::ContestedResourceTask(action)); | ||
| self.show_vote_popup = None; | ||
| } else if ui.button("Vote Deferred").clicked() { | ||
| app_action = AppAction::BackendTask(BackendTask::ContestedResourceTask(action)); | ||
| self.show_vote_popup = None; | ||
| } else if ui.button("Cancel").clicked() { | ||
| self.show_vote_popup = None; | ||
| // Only modify `voters` if `action` is `VoteOnDPNSName` | ||
| if let ContestedResourceTask::VoteOnDPNSName( | ||
| contested_name, | ||
| vote_choice, | ||
| mut voters, | ||
| ) = action | ||
| { | ||
| // Iterate over the voting identities and create a button for each one | ||
| for identity in voting_identities.iter() { | ||
| let button_label = format!( | ||
| "{}", | ||
| identity | ||
| .alias | ||
| .clone() | ||
| .unwrap_or(identity.identity.id().to_string(Encoding::Base58)) | ||
| ); | ||
| if ui.button(button_label).clicked() { | ||
| // Add the selected identity to the `voters` field | ||
| voters.push(identity.clone().clone()); | ||
|
|
||
| // Create a new `VoteOnDPNSName` task with updated voters | ||
| let updated_action = ContestedResourceTask::VoteOnDPNSName( | ||
| contested_name.clone(), | ||
| vote_choice.clone(), | ||
| voters.clone(), // Updated voters | ||
| ); | ||
|
|
||
| // Pass updated action to BackendTask | ||
| app_action = AppAction::BackendTask( | ||
| BackendTask::ContestedResourceTask(updated_action), | ||
| ); | ||
| self.show_vote_popup_info = None; | ||
| } | ||
| } | ||
|
|
||
| // Vote with all identities | ||
| if ui.button("All").clicked() { | ||
| for identity in voting_identities.iter() { | ||
| voters.push(identity.clone().clone()); | ||
| } | ||
|
|
||
| // Create a new `VoteOnDPNSName` task with all voters | ||
| let updated_action = ContestedResourceTask::VoteOnDPNSName( | ||
| contested_name.clone(), | ||
| vote_choice.clone(), | ||
| voters.clone(), // Updated voters | ||
| ); | ||
|
|
||
| // Pass updated action to BackendTask | ||
| app_action = AppAction::BackendTask(BackendTask::ContestedResourceTask( | ||
| updated_action, | ||
| )); | ||
| self.show_vote_popup_info = None; | ||
| } | ||
| } | ||
|
|
||
| // Add the "Cancel" button | ||
| if ui.button("Cancel").clicked() { | ||
| self.show_vote_popup_info = None; | ||
| } | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Check for empty voting identities and handle accordingly
If voting_identities is empty, the UI will not display any buttons for the user to select an identity, potentially causing confusion.
Consider adding a message to inform the user when no voting identities are available:
if voting_identities.is_empty() {
ui.label("No voting identities available to vote with.");
} else {
// Existing code to display voting buttons
}| let local_qualified_identities = self | ||
| .app_context | ||
| .db | ||
| .get_local_qualified_identities(&self.app_context) | ||
| .unwrap_or_default(); | ||
|
|
There was a problem hiding this comment.
Handle potential errors when fetching local qualified identities
Currently, errors from get_local_qualified_identities are ignored with unwrap_or_default(), which may mask underlying issues. Consider handling the Result properly to ensure robustness.
Here's a suggested change:
let local_qualified_identities = self
.app_context
.db
.get_local_qualified_identities(&self.app_context)
- .unwrap_or_default();
+ .unwrap_or_else(|err| {
+ // Handle the error, e.g., log it or display a message
+ eprintln!("Error fetching identities: {:?}", err);
+ Vec::new()
+ });You might also consider displaying an error message to the user if fetching identities fails.
📝 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 local_qualified_identities = self | |
| .app_context | |
| .db | |
| .get_local_qualified_identities(&self.app_context) | |
| .unwrap_or_default(); | |
| let local_qualified_identities = self | |
| .app_context | |
| .db | |
| .get_local_qualified_identities(&self.app_context) | |
| .unwrap_or_else(|err| { | |
| // Handle the error, e.g., log it or display a message | |
| eprintln!("Error fetching identities: {:?}", err); | |
| Vec::new() | |
| }); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/platform/contested_names/vote_on_dpns_name.rs (2)
22-26: Approve signature change and suggest removing unused parameterThe updated method signature correctly includes the new
votersparameter, which aligns with the PR objective of allowing users to select specific masternodes for voting.However, the
_senderparameter appears to be unused (prefixed with an underscore). Consider removing this parameter if it's no longer needed in the function body.pub(super) async fn vote_on_dpns_name( self: &Arc<Self>, name: &String, vote_choice: ResourceVoteChoice, voters: Vec<QualifiedIdentity>, sdk: Sdk, - _sender: mpsc::Sender<TaskResult>, ) -> Result<(), String> {
Line range hint
27-77: Consider refactoring for improved code organizationWhile the code is well-structured and follows consistent conventions, the
vote_on_dpns_namefunction is quite long and handles multiple responsibilities. Consider refactoring it into smaller, more focused functions to improve readability and maintainability. For example:
- Extract the DPNS contract and document type fetching logic into a separate function.
- Create a helper function for creating and submitting a vote for a single qualified identity.
This refactoring would make the main function more concise and easier to understand at a glance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/platform/contested_names/vote_on_dpns_name.rs (3 hunks)
🧰 Additional context used
🔇 Additional comments (3)
src/platform/contested_names/vote_on_dpns_name.rs (3)
3-3: LGTM: New import for QualifiedIdentityThe new import for
QualifiedIdentityis correctly added and necessary for the updated method signature.
Line range hint
48-74: LGTM: Implementation changes for voting with selected masternodesThe updated implementation correctly iterates over the provided
voters, allowing for more flexible voting as per the PR objectives. The error handling for missing associated voter identities is appropriate and provides a clear error message. The code is well-structured and includes helpful comments explaining the purpose of different sections.
68-72: LGTM: Improved error handling for missing voter identitiesThe new error handling for cases where a qualified identity doesn't have an associated voter identity is well-implemented. The error message is clear and includes the identity ID, which will be helpful for debugging.
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).
HIGH #6: Add tracing::warn to 7 silent identity_id decode continues. All `Err(_) => continue` in load() now log before skipping. Matches the existing pattern for account_type/txid decode failures. HIGH #7: payment_type/status catch-all masks corruption. Unknown payment_type no longer defaults to Received — skips row with tracing::warn. Unknown status no longer defaults to Pending — same treatment. Explicitly matches "pending"/"sent"/"received"/ "confirmed"/"failed". HIGH #8: created_at type mismatch (write i64, load u64). Load now reads as Option<i64> (matching the write cast), clamps negative values to 0 via .max(0), converts explicitly to u64. HIGH #9: Proof decode failure silently degrades asset lock status. When proof_data was present but decode failed AND no legacy fallback columns (islock_data, chain_height) exist, skip the entire row with tracing::error instead of loading a degraded entry with Broadcast status. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
When voting on dpns usernames, you can now select which mn to vote with, or select "all" to vote with all loaded mns.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor