Feature disputes#463
Conversation
WalkthroughThe changes update how the application validates the identity of the involved parties by replacing the use of Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/app/dispute.rs (3)
157-157: Unnecessary clone operation.The
.clone()operation before calling.update(pool)is unnecessary since you don't need to preserve the original order after calling update.- .clone() +
211-218: Remove redundant.clone()calls on String types.The
.clone()calls after.to_string()are unnecessary asto_string()already creates a new String.- let counterpart = is_user_present(pool, counterpart_pubkey.to_string().clone()) + let counterpart = is_user_present(pool, counterpart_pubkey.to_string()) .await .map_err(|cause| MostroInternalErr(ServiceError::DbAccessError(cause.to_string())))?; - let initiator = is_user_present(pool, initiator_pubkey.to_string().clone()) + let initiator = is_user_present(pool, initiator_pubkey.to_string()) .await .map_err(|cause| MostroInternalErr(ServiceError::DbAccessError(cause.to_string())))?;
219-223: Fix typo in variable name and consider using a time library for day calculations.There's a typo in the variable name "couterpart_operating_days" (missing 'n'). Additionally, consider using a time library function to calculate the difference in days for better handling of edge cases.
// Calculate operating days of users let now = Timestamp::now(); let initiator_operating_days = (now.as_u64() - initiator.created_at as u64) / 86400; - let couterpart_operating_days = (now.as_u64() - counterpart.created_at as u64) / 86400; + let counterpart_operating_days = (now.as_u64() - counterpart.created_at as u64) / 86400;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/dispute.rs(3 hunks)src/util.rs(2 hunks)
🔇 Additional comments (3)
src/util.rs (1)
242-275: Documentation formatting improvements look good.The enhanced documentation structure with consistent empty lines between sections makes the code more readable and maintainable.
src/app/dispute.rs (2)
8-8: Appropriate import addition.Adding the
is_user_presentimport is needed for the new user validation functionality.
12-12: Good import addition for the new dispute info structure.Adding
SolverDisputeInfofrom the mostro_core dispute module supports the enhanced dispute handling functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/app/admin_take_dispute.rs (1)
124-166: Comprehensive dispute context collection.The added code collects important information about both parties involved in the dispute, providing the admin with valuable context for resolution.
Fix the typo in variable name:
-let couterpart_operating_days = (now.as_u64() - counterpart.created_at as u64) / 86400; +let counterpart_operating_days = (now.as_u64() - counterpart.created_at as u64) / 86400;And update the parameter name in the
SolverDisputeInfo::newcall accordingly:- couterpart_operating_days, + counterpart_operating_days,src/app/dispute.rs (2)
78-83: Update function documentation to match implementation.The function comment still describes the return type as a tuple, but the implementation now returns only a boolean.
Modify the documentation to reflect the actual return type:
/// Gets information about the counterparty in a dispute. /// -/// Returns a tuple containing: -/// - The counterparty's public key as a String -/// - A boolean indicating if the dispute was initiated by the buyer (true) or seller (false) +/// Returns a boolean indicating if the dispute was initiated by the buyer (true) or seller (false)
109-145: Good extraction of notification logic.Extracting the notification logic into a dedicated function improves code organization and separation of concerns.
Add documentation for the new function and fix the typo in the comment:
/// Sends notifications to both the initiator and counterpart about a dispute /// /// # Arguments /// * `dispute` - The dispute object /// * `msg` - The original message that triggered the dispute /// * `order_id` - The ID of the order under dispute /// * `counterpart_token` - Security token for the counterpart /// * `initiator_token` - Security token for the initiator /// * `counterpart_pubkey` - Public key of the counterpart /// * `initiator_pubkey` - Public key of the initiator async fn notify_dispute_to_users( dispute: &Dispute, msg: &Message, order_id: Uuid, counterpart_token: Option<u16>, initiator_token: Option<u16>, counterpart_pubkey: PublicKey, initiator_pubkey: PublicKey, ) -> Result<(), MostroError> { - // Message to discounterpart + // Message to counterpart enqueue_order_msg( msg.get_inner_message_kind().request_id, Some(order_id),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/admin_take_dispute.rs(4 hunks)src/app/dispute.rs(5 hunks)
🔇 Additional comments (5)
src/app/admin_take_dispute.rs (4)
77-77: Correct key check implementation.The change from
event.rumor.pubkeytoevent.senderfixes a critical issue in the dispute mechanism. This aligns with the PR objectives of restoring dispute functionality by correcting key checks.
85-96: Improved order retrieval with proper error handling.The implementation now uses a more explicit
Order::by_idapproach with comprehensive error handling, which is more robust than the previousget_ordermethod. This helps catch specific error cases like missing orders and database access errors.
174-174: Enhanced payload with dispute information.The addition of
dispute_infoto the payload provides the admin with comprehensive context about the dispute, including user ratings and operating history, which will help in making informed decisions.
185-185: Consistent naming with admin_pubkey.Renaming from
solver_pubkeytoadmin_pubkeyimproves code clarity and consistency, aligning with the actual role of the entity taking control of the dispute.src/app/dispute.rs (1)
208-237: Improved public key handling.The refactored code for obtaining public keys is more explicit and easier to follow, and the use of the new notification function simplifies the main dispute handling logic.
|
@Catrya when you can do a round of tests, now:
I will try at home asap too. NOTE: to use this mostrod version you need the branch of this pull request of mostro-core locally using the cargo.toml patch at bottom: [patch.crates-io]
mostro-core = { path = "path/to/mostro-core" } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/app/admin_take_dispute.rs (2)
20-72: Minor spelling fix & defensive check suggestion.
- There's a small misspelling in the variable name
couterpart_operating_days(line 60).- Consider adding a defensive check to ensure user creation times are not in the future, which could lead to incorrect operating days calculations.
A sample fix for the spelling:
- let couterpart_operating_days = (now.as_u64() - counterpart.created_at as u64) / 86400; + let counterpart_operating_days = (now.as_u64() - counterpart.created_at as u64) / 86400;
176-176: Possible naming clarification.Naming it
admin_pubkeymay be misleading if the dispute is taken by a solver who is not strictly an admin. Consider renaming it to reflect the role more precisely.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/admin_take_dispute.rs(2 hunks)
🔇 Additional comments (12)
src/app/admin_take_dispute.rs (12)
1-1: No issues with new import usage.
5-5: New imports for dispute objects look good.
12-12: Order import is consistent with the new dispute logic.
131-131: Admin/solver verification logic appears sound.
139-139: Order retrieval aligns with the new dispute ID usage.
144-144: Assigning solver_pubkey is correct for dispute tracking.
147-147: Log message is helpful for troubleshooting.
156-157: Logic for preparing solver info message is properly invoked.
165-165: Payload now includes SolverDisputeInfo for the dispute.
171-171: Direct message to the event sender looks correct.
182-182: Payload with cloned peer is consistent with prior usage.
190-190: Payload with peer references is correct for the second message.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/app/admin_take_dispute.rs (2)
20-72: Well-structured new function for user data retrievalThe new
prepare_solver_info_messagefunction correctly handles the complexity of retrieving user information for both parties involved in a dispute. It also provides proper error handling and clear logic flow.However, there's a typo in a variable name.
- let couterpart_operating_days = (now.as_u64() - counterpart.created_at as u64) / 86400; + let counterpart_operating_days = (now.as_u64() - counterpart.created_at as u64) / 86400;Also, don't forget to update the parameter in line 68:
- couterpart_operating_days, + counterpart_operating_days,
151-151: Added clone() before database updateThe addition of
.clone()before updating the dispute in the database indicates ownership requirements for the update method. While this addresses ownership issues, consider if this is the most efficient approach.If the
updatemethod doesn't need to consumeself, consider modifying it to take a reference instead of ownership to avoid the need for cloning, especially ifDisputeis a large struct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/admin_take_dispute.rs(2 hunks)
🔇 Additional comments (11)
src/app/admin_take_dispute.rs (11)
1-1: Appropriate addition of required importsAdding the
is_user_presentimport supports the new functionality for retrieving user information for disputes.
5-6: Good import organization for new functionalityAdding the
SolverDisputeInfoandOrderimports is necessary for the new functionality that prepares detailed information for dispute solvers.Also applies to: 12-12
81-85: Updated logging with improved terminologyThe log message now correctly refers to "admin pubkey" which better reflects the authorization context in this function.
131-131: Fixed key verification to use proper sender identificationChanged from using
event.rumor.pubkeytoevent.senderfor properly identifying the actor who is attempting to take a dispute. This aligns with the PR objective to fix incorrect key checks.
144-144: Fixed public key assignmentThe assignment of
solver_pubkeynow correctly uses theevent.senderproperty for consistent identity handling.
147-148: Improved logging with consistent identity referenceUpdated log message to use
event.senderinstead ofevent.rumor.pubkeyfor consistent identity handling throughout the code.
156-158: Added solver information message preparationThis new section properly prepares the detailed information that will be sent to the solver, fulfilling the PR objective to add initiator and counterpart information.
165-165: Enhanced dispute payload with detailed informationThe payload now includes the
dispute_infoobject that contains the ratings and operating days for both parties, which improves the information available to the dispute solver.
171-171: Fixed message recipientNow correctly using
event.senderfor the direct message recipient, ensuring the message reaches the intended user.
176-176: Improved variable naming and fixed public key referenceRenamed variable from
solver_pubkeytoadmin_pubkeyto better reflect its role, and correctly usedevent.senderfor consistent identity handling.
182-182: Updated payload referencesCorrectly updated the payload to use the renamed
admin_pubkeyvariable, maintaining consistency throughout the code.Also applies to: 190-190
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/app/admin_take_dispute.rs (1)
20-72: Good addition: Enhanced dispute information for solversThis new function provides valuable context to dispute solvers by retrieving user ratings and operational history. This will help in making more informed dispute resolutions.
However, there's a typo in variable naming:
- let couterpart_operating_days = (now.as_u64() - counterpart.created_at as u64) / 86400; + let counterpart_operating_days = (now.as_u64() - counterpart.created_at as u64) / 86400;And update the variable name in the function call as well:
let dispute_info = SolverDisputeInfo::new( order, dispute, &counterpart, &initiator, initiator_operating_days, - couterpart_operating_days, + counterpart_operating_days, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/admin_take_dispute.rs(4 hunks)
🔇 Additional comments (9)
src/app/admin_take_dispute.rs (9)
1-1: Import changes correctly support new functionalityThe additional imports for
is_user_present,SolverDisputeInfo, andOrderproperly support the new dispute information functionality.Also applies to: 5-5, 12-12
81-85: Logging improvementThe updated logging message correctly identifies the admin pubkey in the output, improving clarity.
132-132: Security enhancement: Using direct sender verificationSwitching from
event.rumor.pubkeytoevent.senderimproves security by using the authenticated sender identity for authorization checks.
140-142: Clean order retrievalThe order retrieval is now more straightforward and properly uses the existing utility function.
145-145: Consistent identity handlingUsing
event.senderforsolver_pubkeymaintains consistency with the authorization check.
158-159: Enhanced solver information in dispute messagesIncluding the dispute information in the message payload provides valuable context to the solver, improving the dispute resolution process.
Also applies to: 166-166
172-184: Consistent use of sender identityThe sender's identity is now consistently used from
event.senderthroughout the message creation and sending process.
186-201: Simplified key handling for buyer and seller messagesThe code now correctly uses a reference to
mostro_keysand has more straightforward error handling for getting buyer and seller public keys.
152-155:Details
✅ Verification successful
Consider removing unnecessary clone
The clone operation before the update call may be unnecessary and could be eliminated for better performance.
- dispute - .clone() - .update(pool) + dispute + .update(pool)
🏁 Script executed:
#!/bin/bash # Check if Dispute::update() consumes self or takes a reference rg -A 3 "fn update" --type=rustLength of output: 1608
Remove unnecessary clone for performance improvement
It appears that the
update()method, similar to other update functions in the codebase, is designed to operate on a reference rather than consuming the whole object. Based on the grep results comparing update method signatures across the repository, the extra.clone()call before invokingupdate(pool)seems unnecessary. Removing the clone should simplify the code and avoid the overhead of duplicating the dispute instance.Changes to consider:
- File:
src/app/admin_take_dispute.rs(Lines: 152-155)- Diff:
- dispute - .clone() - .update(pool) + dispute + .update(pool)
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/app/admin_take_dispute.rs (1)
172-186:⚠️ Potential issueFixed key handling in message sending
Now correctly using the provided
mostro_keysfor signing messages to users rather than creating new keys, ensuring consistent identity across communications.
🧹 Nitpick comments (3)
src/app/admin_take_dispute.rs (3)
20-72: Well-structured function to gather participant informationThis new function effectively retrieves and organizes necessary information for dispute resolution, including user ratings and operational history. The implementation uses proper error handling for database access and key validation.
There's a small typo in variable naming:
- let couterpart_operating_days = (now.as_u64() - counterpart.created_at as u64) / 86400; + let counterpart_operating_days = (now.as_u64() - counterpart.created_at as u64) / 86400;Also, ensure the variable with the typo is correctly passed to the
SolverDisputeInfo::new()function.
137-138: Unnecessary clone operation before database updateThe
.clone()call before.update()is unnecessary as the update method could take a reference instead.- dispute - .clone() - .update(pool) + dispute + .update(pool)
20-240: Consider adding race condition protection for dispute claimingThe current implementation might allow multiple solvers to claim the same dispute if they attempt it simultaneously, as there's no optimistic locking or transaction mechanism in place.
Consider implementing a transaction or optimistic locking approach:
// Pseudo-code for a transaction-based approach let mut tx = pool.begin().await?; // Get dispute with the current version let mut dispute = get_dispute_with_version(&msg, &tx).await?; // Check version hasn't changed since retrieval if !check_dispute_version(dispute.version) { tx.rollback().await?; return Err(MostroCantDo(CantDoReason::DisputeAlreadyTaken)); } // Update dispute dispute.status = Status::InProgress.to_string(); dispute.solver_pubkey = Some(event.sender.to_string()); dispute.taken_at = Timestamp::now().as_u64() as i64; dispute.version += 1; // Save it to DB within transaction dispute.update(&tx).await?; // Commit transaction tx.commit().await?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app.rs(1 hunks)src/app/admin_take_dispute.rs(6 hunks)src/util.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/util.rs
🔇 Additional comments (7)
src/app.rs (1)
234-234: Function signature now correctly includes the key parameterThis update ensures that
admin_take_dispute_actionnow receives themy_keysparameter, which is consistent with other admin actions and necessary for proper message signing. This alignment fixes a key inconsistency in the dispute handling flow.src/app/admin_take_dispute.rs (6)
1-13: Added necessary imports for enhanced dispute functionalityThe updated imports support the new functionality for retrieving dispute information and user data. The addition of
is_user_presentandSolverDisputeInfoenables the new dispute information flow.
81-85: Improved logging for admin vs event public keysThe updated logging provides clearer information about which keys are being compared in the dispute resolution flow, helping with debugging.
113-113: Enhanced dispute retrieval mechanismChanged to use the dedicated
get_disputefunction instead of direct message field access, improving the code's structure and reusability.
130-134: Correctly recording solver public key and improving loggingThis change properly records the dispute solver's public key and enhances logging with relevant dispute information.
142-144: Added comprehensive dispute information for solversThis enhancement provides solvers with crucial context about the dispute participants, including reputation and operational history, which should lead to more informed resolution decisions.
151-152: Updated payload to include richer dispute contextThe message now includes the
SolverDisputeInfoin the payload, giving solvers access to comprehensive information about the participants involved in the dispute.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/app/admin_take_dispute.rs (1)
117-117:⚠️ Potential issueInconsistent key validation
While most of the code now uses
event.senderfor validating the user's identity, this line still usesevent.rumor.pubkey. This inconsistency could cause authentication issues.- if !pubkey_event_can_solve(pool, &event.rumor.pubkey, dispute_status).await { + if !pubkey_event_can_solve(pool, &event.sender, dispute_status).await {
🧹 Nitpick comments (1)
src/app/admin_take_dispute.rs (1)
20-72: Well-structured implementation of solver info preparationThe new function encapsulates the logic for retrieving user ratings and calculating operating days in a clean, modular way. Good error handling throughout.
However, there's a typo in variable name:
- let couterpart_operating_days = (now.as_u64() - counterpart.created_at as u64) / 86400; + let counterpart_operating_days = (now.as_u64() - counterpart.created_at as u64) / 86400;Also, ensure this variable is correctly passed to the
SolverDisputeInfo::newfunction (line 68).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/admin_take_dispute.rs(7 hunks)
🔇 Additional comments (16)
src/app/admin_take_dispute.rs (16)
1-1: Appropriate addition of import for user validationAdding
is_user_presentis a good choice for the new functionality that checks if users exist in the database.
3-3: Good improvement to importsAdding
get_disputeto the imports aligns with the PR objective of fixing dispute functionality by using a more consistent approach to retrieve dispute data.
5-5: Proper import of SolverDisputeInfoAdding the import for
SolverDisputeInfosupports the new functionality to provide detailed dispute information to solvers.
12-12: Correctly added Order importThe Order import is properly added to support the new function that requires order details.
81-85: Improved logging for key validationThe updated logging provides better visibility into the key comparison process, which is helpful for debugging identity verification issues.
113-113: Fixed dispute retrievalUsing the
get_disputefunction from the utility module creates a more consistent approach to dispute retrieval, addressing the PR's goal of fixing dispute functionality.
130-130: Fixed key validation using correct sender fieldThis change is critical for proper identity verification. Using
event.senderinstead ofevent.rumor.pubkeyensures that the identity check is performed against the actual sender of the request.
133-133: Updated logging with correct identity fieldThe logging now properly records the sender's key instead of the rumor pubkey, providing more accurate audit trails.
143-143: Good modularization of solver info preparationExtracting the preparation of solver information to a dedicated function improves code organization and readability.
151-151: Enhanced dispute information payloadNow sending more detailed solver dispute information instead of just the basic dispute data, which should help solvers make more informed decisions.
157-157: Fixed DM recipient to use correct sender fieldUsing
event.senderensures messages are sent to the actual sender of the request rather than a potentially different pubkey from the rumor.
168-168: Fixed peer payload with correct identityThe peer payload now correctly uses the sender's public key hexadecimal representation rather than potentially incorrect rumor pubkey.
173-173: Properly getting buyer pubkeyUsing the order's getter method ensures consistent access to the buyer's public key, improving reliability.
184-185: Properly getting seller pubkey and using mostro keysUsing the order's getter method for the seller's public key and the provided mostro keys ensures consistent identity handling.
106-106: Added and properly used mostro_keys parameterThe function now takes a
mostro_keysparameter which is consistently used throughout the function for sending messages to parties, improving key management.Also applies to: 173-174, 185-186
234-234: Improved error loggingError logging now includes the dispute ID, which will help with debugging failed event sends.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/app/admin_take_dispute.rs (2)
60-60: Fix typo in variable nameThere's a typo in the variable name:
couterpart_operating_daysshould becounterpart_operating_days.- let couterpart_operating_days = (now.as_u64() - counterpart.created_at as u64) / 86400; + let counterpart_operating_days = (now.as_u64() - counterpart.created_at as u64) / 86400;
62-69: Update variable name in the function callMake sure to update the variable name in the
SolverDisputeInfo::newcall to match the corrected name.let dispute_info = SolverDisputeInfo::new( order, dispute, &counterpart, &initiator, initiator_operating_days, - couterpart_operating_days, + counterpart_operating_days, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/admin_take_dispute.rs(6 hunks)
🔇 Additional comments (10)
src/app/admin_take_dispute.rs (10)
20-72: Well-structured function for preparing solver informationThe new
prepare_solver_info_messagefunction is a good addition that encapsulates the logic for retrieving user ratings and calculating operating days. It handles errors appropriately and uses the proper user identity verification.
117-117: Improved security with correct identity verificationCorrectly using
event.senderinstead ofevent.rumor.pubkeyfor identity verification ensures that the proper security checks are performed when handling disputes.
130-131: Consistent use of event.sender for solver identificationGood update to use
event.senderfor the solver_pubkey assignment, maintaining consistency with the identity verification approach.
142-143: Enhanced dispute information handlingThe integration of the
prepare_solver_info_messagefunction provides more comprehensive information about users involved in disputes, which will be valuable for dispute resolution.
151-151: Improved payload structure with dispute infoThe payload now includes detailed solver dispute information, which aligns with the PR objective of restoring dispute functionality.
168-169: Proper peer information in messagingUsing
event.sender.to_hex()for the Peer information ensures that the correct solver identity is communicated to the users.
173-174: Update to use mostro_keys.clone() instead of creating new KeysGood refactoring to use the provided
mostro_keysparameter instead of retrieving keys again, which improves efficiency and reduces duplication.
185-186: Consistent key usage across message sendingThe consistent use of
mostro_keys.clone()and reusing themsg_to_usersvariable shows good code reuse and maintainability.
224-225: Improved error loggingThe log message now properly includes the dispute ID, which will make debugging easier.
234-234: Enhanced error logging for failed eventsThe log message clearly identifies which dispute failed to send a status event, improving troubleshooting capabilities.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/app/admin_take_dispute.rs (1)
20-77: New function adds valuable solver context, but contains a typoThe
prepare_solver_info_messagefunction is a well-structured addition that gathers important user statistics to help solvers make informed decisions about disputes.However, there's a typo in the variable name at line 65 (
couterpart_operating_daysshould becounterpart_operating_days), which is carried through to line 73.-let couterpart_operating_days = (now.as_u64() - counterpart.created_at as u64) / 86400; +let counterpart_operating_days = (now.as_u64() - counterpart.created_at as u64) / 86400; let dispute_info = SolverDisputeInfo::new( order, dispute, &counterpart, &initiator, initiator_operating_days, - couterpart_operating_days, + counterpart_operating_days, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
Cargo.toml(2 hunks)src/app/admin_take_dispute.rs(6 hunks)
🔇 Additional comments (12)
Cargo.toml (1)
40-40: Dependency version update aligned with dispute feature fixesThe version update of
mostro-corefrom 0.6.30 to 0.6.31 supports the dispute functionality fixes mentioned in the PR description. This update likely includes the necessary support for new structures likeSolverDisputeInfoused in this PR.src/app/admin_take_dispute.rs (11)
1-19: Improved imports for enhanced dispute handlingThe updated imports properly support the extended dispute functionality, including the addition of
is_user_presentfrom the database module andSolverDisputeInfofrom the core module.
86-90: Enhanced logging for better debuggingThe improved logging provides better context about the admin and event pubkeys being compared, which will help in troubleshooting authentication issues.
118-118: Using dedicated utility function for dispute retrievalReplacing direct message extraction with the
get_disputeutility function improves code organization and maintainability.
122-122: Fixed key validation using correct sender fieldThe change from
event.rumor.pubkeytoevent.senderensures that the identity check is performed against the actual sender of the request, addressing one of the key issues mentioned in the PR description.
131-138: Improved order retrieval with better error handlingThe enhanced order retrieval logic with explicit error handling makes the code more robust and provides clearer error messages.
142-146: Proper solver attribution and loggingUsing
event.senderfor the solver's pubkey ensures correct attribution of the dispute to the solver, and the updated logging provides better traceability.
154-156: Enhanced solver information with contextual user dataThe addition of
SolverDisputeInfoprovides solvers with valuable context about the parties involved in the dispute, including their ratings and platform experience, which helps in making fair decisions.
163-164: Enriched payload with dispute contextThe updated payload structure now includes comprehensive dispute information, improving the solver's ability to understand and resolve the case efficiently.
175-181: Clear communication of dispute handler to usersThe message now properly informs both parties about who will assist them with the dispute, using the correct sender's public key.
184-186: Proper use of mostro_keys for message authenticationThe change ensures that messages are sent with the correct keys, maintaining proper authentication and message integrity.
196-198: Consistent key usage for both buyer and seller communicationsThe symmetrical use of keys and message structures for both buyer and seller ensures consistent communication patterns.
| let (initiator_pubkey, counterpart_pubkey) = if order.buyer_dispute { | ||
| ( | ||
| &order | ||
| .get_master_buyer_pubkey() | ||
| .map_err(|_| MostroInternalErr(ServiceError::InvalidPubkey))?, | ||
| &order | ||
| .get_master_seller_pubkey() | ||
| .map_err(|_| MostroInternalErr(ServiceError::InvalidPubkey))?, | ||
| ) | ||
| } else { | ||
| ( | ||
| &order | ||
| .get_master_seller_pubkey() | ||
| .map_err(|_| MostroInternalErr(ServiceError::InvalidPubkey))?, | ||
| &order | ||
| .get_master_buyer_pubkey() | ||
| .map_err(|_| MostroInternalErr(ServiceError::InvalidPubkey))?, | ||
| ) | ||
| }; | ||
|
|
||
| info!( | ||
| "initiator pubkey: {} - counterpart pubkey: {}", | ||
| initiator_pubkey, counterpart_pubkey | ||
| ); | ||
|
|
||
| // Get users ratings | ||
| // Get counter to vote from db | ||
|
|
||
| let counterpart = is_user_present(pool, counterpart_pubkey.to_string()) | ||
| .await | ||
| .map_err(|cause| MostroInternalErr(ServiceError::DbAccessError(cause.to_string())))?; | ||
|
|
||
| let initiator = is_user_present(pool, initiator_pubkey.to_string()) | ||
| .await | ||
| .map_err(|cause| MostroInternalErr(ServiceError::DbAccessError(cause.to_string())))?; | ||
|
|
||
| // Calculate operating days of users | ||
| let now = Timestamp::now(); | ||
| let initiator_operating_days = (now.as_u64() - initiator.created_at as u64) / 86400; | ||
| let couterpart_operating_days = (now.as_u64() - counterpart.created_at as u64) / 86400; | ||
|
|
||
| let dispute_info = SolverDisputeInfo::new( | ||
| order, | ||
| dispute, | ||
| &counterpart, | ||
| &initiator, |
There was a problem hiding this comment.
I think that Mostro should send the admin the trade keys of the initiator and the counterpart, not the 0 pub keys
initiator_pubkey: "355547af841fe7fe09159b0ef891a9b30d32fea009d12c2c84b8435f143cdd24",
buyer_pubkey: Some(
"ec41d3b190807d4346bf6dfff8083b40279cae1ab43b0acefec85c990574820a",
),
buyer_token: Some(
212,
),
seller_pubkey: Some(
"1a0b899a006b9fffd5f096d51880321776c0da8fca063e7c6192ffb85389cff4",
),
seller_token: Some(
606,
),
In addition, having the 0 pubkey, the admin cannot identify if it is the buyer or the seller
There was a problem hiding this comment.
probably i am getting the masters key will check tomorrow...
|
I am looking back to the code and probably you can test it using the take admin command from CLI using the @Catrya @grunch
Tell me your opinions about that before I finish the full privacy case! :) |
|
I am preparing a versione with full control in this way:
|
|
@Catrya i implemented the logic where one user can be in full privacy mode and the other no ( clearly also both or none ). Let me know if you will test it and feel free to write doubts also on tg |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/app/admin_take_dispute.rs (2)
35-121: Fix typo in variable name.There's a typo in the variable name on line 108:
- let couterpart_operating_days = (now.as_u64() - counterpart.created_at as u64) / 86400; + let counterpart_operating_days = (now.as_u64() - counterpart.created_at as u64) / 86400;Also, remember to update the variable reference in the SolverDisputeInfo constructor call on line 117:
- couterpart_operating_days, + counterpart_operating_days,
229-230: Avoid unnecessary cloning of Keys.The
mostro_keysparameter is already passed as a reference, but it's being cloned unnecessarily.- mostro_keys.clone(), + mostro_keys,Same change should be applied to line 241.
Also applies to: 241-242
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/admin_take_dispute.rs(6 hunks)
🔇 Additional comments (8)
src/app/admin_take_dispute.rs (8)
21-33: Well-implemented privacy detection logic.This function clearly identifies users in full privacy mode by comparing their regular public key with their master public key. The code is concise and the comments are helpful in understanding the purpose.
53-103: Good handling of full privacy cases.The logic for determining the initiator and counterpart is well implemented. The code effectively checks if users are in full privacy mode and retrieves their data accordingly, falling back to default User objects when necessary.
161-162: Improved dispute retrieval.Using the
get_disputeutility function simplifies the code and provides better error handling.
166-166: Fixed key validation using correct sender field.This change is critical for proper identity verification. Using
event.senderinstead ofevent.rumor.pubkeyensures that the identity check is performed against the actual sender of the request.
174-182: Improved error handling for order retrieval.The conditional retrieval with proper error handling ensures that invalid order IDs are caught and reported correctly.
186-190: Consistent use of event.sender.The code now consistently uses
event.senderfor the solver's public key, which aligns with the fixed key validation approach.
198-199: Well-structured solver information message preparation.Using a dedicated function to prepare the solver information message improves code organization and separation of concerns.
187-187:Details
✅ Verification successful
Consider using u64 for timestamps.
Converting the timestamp to i64 might lead to issues in 2038 (Year 2038 problem) for long-running systems.
Consider whether using u64 would be more appropriate here, or if there's a specific reason for using i64.
🌐 Web query:
What are the implications of storing Unix timestamps as i64 vs u64 in Rust?💡 Result:
Storing Unix timestamps as
i64oru64in Rust involves trade-offs in date range coverage, compatibility with libraries, and handling of historical dates. Here's a structured analysis:Supported Date Ranges
i64:
- Supports dates before 1970 via negative values.
- Example:
-2208936075_000_000_000maps to1900-01-01 14:38:45 UTC[1][4].- With nanosecond precision, spans ±584 years relative to 1970 [1].
u64:
- Limited to dates ≥1970.
- Common for systems that don't require pre-epoch timestamps.
Compatibility & Interoperability
- Chrono and Standard Libraries:
i64aligns with Rust’sSystemTime(which usesi64internally viaDuration) and thechronocrate, which returnstimestamp()asi64for UNIX epoch conversions [1][4][9].- Database/API Constraints:
Systems expectingi64(e.g., some databases) may require convertingu64values, risking overflow if timestamps exceedi64::MAX(~9.2e18 nanoseconds or ±292 billion years) [7][13].Arithmetic and Conversions
- Overflow Handling:
u64operations (e.g., subtraction) risk underflow panics in debug mode.i64avoids this for pre-epoch calculations.- Bitwise Casting:
Convert betweeni64/u64losslessly usingcast_signed()andcast_unsigned()(nightly Rust) orto_ne_bytes()for stable code [7][8]:// Bit-preserving conversion let u_val: u64 = i_val.to_ne_bytes().try_into().unwrap();Storage and Serialization
- Size: Both use 8 bytes.
- Efficiency:
i64is preferred in formats likeiso8601-timestamp, which serialize Unix timestamps asi64milliseconds for compactness [3].Key Considerations
Factor i64u64Pre-1970 Dates Supported Not Supported Library Compatibility Native (chrono, std::time) Requires conversions Overflow Risk Only if exceeding ±584 years On underflow (pre-1970 access) Serialization Compact with negative handling Simpler for post-1970 systems Recommendations
- Use
i64if:
- Historical (pre-1970) dates are needed.
- Interfacing with Rust’s time libraries (e.g.,
chrono).- Use
u64if:
- Guaranteed post-1970 timestamps.
- Interoperating with systems enforcing unsigned values.
For most cases,
i64is more flexible and avoids edge cases, as it’s widely adopted in Rust’s time-handling ecosystem.Citations:
- 1: https://docs.rs/chrono/latest/chrono/struct.DateTime.html
- 2: https://users.rust-lang.org/t/unix-timestamp-in-nanoseconds/73926
- 3: https://docs.rs/iso8601-timestamp
- 4: chronotope/chrono#326
- 5: https://doc.rust-lang.org/std/primitive.u64.html
- 6: https://blog.rust-lang.org/2024/03/30/i128-layout-update.html
- 7: https://users.rust-lang.org/t/i64-u64-mapping-revisited/109315
- 8: https://doc.rust-lang.org/std/primitive.i64.html
- 9: https://doc.rust-lang.org/std/time/struct.SystemTime.html
- 10: https://doc.rust-lang.org/std/?search=saturating
- 11: https://internals.rust-lang.org/t/proposal-integer-conversion-methods/9355
- 12: https://users.rust-lang.org/t/are-there-any-negative-implications-of-using-u64-in-wasm32/57419
- 13: chronotope/chrono#431
- 14: https://web.mit.edu/rust-lang_v1.25/arch/amd64_ubuntu1404/share/doc/rust/html/std/primitive.u64.html
- 15: https://internals.rust-lang.org/t/duration-as-milliseconds/7605?page=3
- 16: https://web.mit.edu/rust-lang_v1.25/arch/amd64_ubuntu1404/share/doc/rust/html/std/primitive.i64.html
- 17: https://internals.rust-lang.org/t/duration-as-milliseconds/7605?page=2
- 18: https://paritytech.github.io/polkadot-sdk/master/polkadot_sdk_frame/traits/trait.Zero.html
Timestamp Numeric Type Assessment
- The conversion from
u64toi64in the current code is safe and in line with common Rust practices—Rust’s time libraries (e.g.,chronoandstd::time) typically usei64to support both pre- and post-1970 dates.- The Year 2038 problem is a concern for 32-bit timestamps, not for 64-bit integers like
i64. Therefore, the conversion does not introduce a 2038 risk.- If your application is guaranteed to work only with post-1970 dates, you might consider storing timestamps as
u64directly to avoid the explicit cast. However, retainingi64offers greater flexibility by accommodating historical dates.Based on these points, please confirm whether the current implementation meets your system’s requirements or if switching to
u64would clarify intent.
The case I was always trying and didn't work was when the seller or buyer were in privacy mode. I never tried the solver in privacy mode. Sorry if I confused you.
I had understood that in privacy mode, users would be in the database, but each trade key, since it's not related to an identity key, would be in a new row as if it were another identity key. It's more private because multiple trades keys can't be related to each other, not because it's not in the database. If it's that way, when the information is sent, the reputation and other data would be 0 because it doesn't have one, just as if it were a new user. |
Nooooo! you did not confuse me...now do normal tests, what I did not think about that it could be quite normal that the make is full privacy and taker no or viceversa. So now I took in consideration these cases:
No users id key or trade key are saved in users table, they are saved only in when they are in rating ( normal ) mode. |
…s is in preparation for migrating to nostr-sdk 0.40
… 0.40 when we will fix signature issue
|
@Catrya could you please do a round of test with latest commit? I rolled back sdk version to 0.38 while we are investigating the cause of the error in signature. You can test with the relative pull-request of mostro-core with same roll back. [patch.crates-io]
mostro-core = { git = "https://github.com/MostroP2P/mostro-core", branch = "rollback-sdk-0-38" }With @grunch we spoke about this and decided to merge to have disputes working, so with this rollback we should have all ready for merge. |
* 📝 Add docstrings to `feature-full-privacy-mode-checks` (#455) * add feature to check correctly if order is full privacy or normal * 📝 Add docstrings to `feature-full-privacy-mode-checks` Docstrings generation was requested by @arkanoider. * #454 (comment) The following files were modified: * `src/app/order.rs` * `src/util.rs` --------- Co-authored-by: arkanoider <github.913zc@simplelogin.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: arkanoider <113362043+arkanoider@users.noreply.github.com> * Fix network in order event (#452) Now in orers event 38383 the network that comes out is the correct one * Feature anyhow removal (#459) * remove of anyhow dependency start * removed anyhow dependecy * remove patch from cargo.toml * Update mostro-core version --------- Co-authored-by: Francisco Calderón <fjcalderon@gmail.com> * Feature disputes (#463) * Feature: dispute fix for new logic * fix for coop cancel case * Add logic for info message about dispute for solver * first working dispute info message for admin after taking disputes * added fields to dispute info message for solver * removed some probably useless and refactored a bit admin-take-dispute function * clippy suggestion * other cosmetics to admin take file * some other cosmetics on admin-take-dispute file * fix on admin-take * Update admin_take_dispute.rs * fixed a wrong search of order id in admin take file@ * Fix: added correct check to add solver from admin cli * fix: wrong check on admin cancel * Fix initiator pubkey: now is the trade key of initiator not identity * quick fix for full privacy orders * new logic with mixed full privacy - regular reputation mode * Update cargo toml to compile with version 0.6.32 of mostro-core - this is in preparation for migrating to nostr-sdk 0.40 * rollback sdk to 0.38 version to merge disputes - we will come back to 0.40 when we will fix signature issue * Update mostro-core dependency --------- Co-authored-by: Francisco Calderón <fjcalderon@gmail.com> * added correction for the case of buyer adding back a new invoice after payment failure (#462) * Fix for nostr sdk 40 issue on incoming message (#465) * testing sdk 40 * fix for sdk 40 * cleaned cargo.toml * Bump mostro core version --------- Co-authored-by: Francisco Calderón <fjcalderon@gmail.com> * fix: sends order with an updated 'status' field as the reply to add-invoice when there is a preimage (#464) * Feature-nip69-order-status (#467) * feature: align to nip69 order status in nostr event * Improved in progress nip69 logic * refined event states of the order with nip69 request * add another check to avoid multiple events with in-progress state * Privacy range order fix (#468) * add optional field for users in full privacy inside solver message * To be tested - privacy range child order fix * fix cargo.toml * bumped mostro-core version to 6.35 * New user or privacy order send same user info with zeros (#471) * new user or privacy order send same user info with zeros * fix days field and removed some println! macro with nicer tracing messages * bumped mostro-core version to 6.36 and added requested fix to compile * Add previous order state in database for solver message (#472) * add previous state in database for solver message * bumped mostro-core version to 6.36 * Taker info message to maker. I think that could be modifies in a more generic UserInfo. (#473) * added logic to send infos to maker when an order is taken * Add takers info message for maker * fix cargo.toml * set peer pubkey to empty string when notifying taker reputation to maker * moved user info message to the right point of the order flow * fix cargo.toml * changed message struct from UserDisputeInfo to UserInfo * fix: get master keys for user infos * Bumped mostro-core to version 6.38 * fix for buy order flow with message to maker (#479) * Rabbit fixes --------- Co-authored-by: Francisco Calderón <fjcalderon@gmail.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Catrya <140891948+Catrya@users.noreply.github.com> Co-authored-by: Bilthon <bilthon@gmail.com>
* Feature: dispute fix for new logic * fix for coop cancel case * Add logic for info message about dispute for solver * first working dispute info message for admin after taking disputes * added fields to dispute info message for solver * removed some probably useless and refactored a bit admin-take-dispute function * clippy suggestion * other cosmetics to admin take file * some other cosmetics on admin-take-dispute file * fix on admin-take * Update admin_take_dispute.rs * fixed a wrong search of order id in admin take file@ * Fix: added correct check to add solver from admin cli * fix: wrong check on admin cancel * Fix initiator pubkey: now is the trade key of initiator not identity * quick fix for full privacy orders * new logic with mixed full privacy - regular reputation mode * Update cargo toml to compile with version 0.6.32 of mostro-core - this is in preparation for migrating to nostr-sdk 0.40 * rollback sdk to 0.38 version to merge disputes - we will come back to 0.40 when we will fix signature issue * Update mostro-core dependency --------- Co-authored-by: Francisco Calderón <fjcalderon@gmail.com>
* 📝 Add docstrings to `feature-full-privacy-mode-checks` (#455) * add feature to check correctly if order is full privacy or normal * 📝 Add docstrings to `feature-full-privacy-mode-checks` Docstrings generation was requested by @arkanoider. * #454 (comment) The following files were modified: * `src/app/order.rs` * `src/util.rs` --------- Co-authored-by: arkanoider <github.913zc@simplelogin.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: arkanoider <113362043+arkanoider@users.noreply.github.com> * Feature disputes (#463) * Feature: dispute fix for new logic * fix for coop cancel case * Add logic for info message about dispute for solver * first working dispute info message for admin after taking disputes * added fields to dispute info message for solver * removed some probably useless and refactored a bit admin-take-dispute function * clippy suggestion * other cosmetics to admin take file * some other cosmetics on admin-take-dispute file * fix on admin-take * Update admin_take_dispute.rs * fixed a wrong search of order id in admin take file@ * Fix: added correct check to add solver from admin cli * fix: wrong check on admin cancel * Fix initiator pubkey: now is the trade key of initiator not identity * quick fix for full privacy orders * new logic with mixed full privacy - regular reputation mode * Update cargo toml to compile with version 0.6.32 of mostro-core - this is in preparation for migrating to nostr-sdk 0.40 * rollback sdk to 0.38 version to merge disputes - we will come back to 0.40 when we will fix signature issue * Update mostro-core dependency --------- Co-authored-by: Francisco Calderón <fjcalderon@gmail.com> * Fix for nostr sdk 40 issue on incoming message (#465) * testing sdk 40 * fix for sdk 40 * cleaned cargo.toml * Bump mostro core version --------- Co-authored-by: Francisco Calderón <fjcalderon@gmail.com> * Feature-nip69-order-status (#467) * feature: align to nip69 order status in nostr event * Improved in progress nip69 logic * refined event states of the order with nip69 request * add another check to avoid multiple events with in-progress state * Taker info message to maker. I think that could be modifies in a more generic UserInfo. (#473) * added logic to send infos to maker when an order is taken * Add takers info message for maker * fix cargo.toml * set peer pubkey to empty string when notifying taker reputation to maker * moved user info message to the right point of the order flow * fix cargo.toml * changed message struct from UserDisputeInfo to UserInfo * fix: get master keys for user infos * Bumped mostro-core to version 6.38 * first commit for database encryption * Moving on with encryption work * Implemented base64 on cipher * Takebuy and takesell pending orders check now working with encryption * Removed unwraps * Finishing code: changed password management and completing checks * Finishing code: used hashset for search * added unit test for evaluate timings * first order cycle completed - to be fixed empty password case * Added logic for testing a caching idea using hashmap to speed up search of identity keys when encrypted * ready to start review process * Cleaned test * removed patch from cargo.toml * Rabbit fixes * some improvements to have also rating working with encryption * Removed useless call in db.rs * fix for tests * Cosmetics on release.rs - improved a bit a function in more ribust way * fix: wrong check on full privacy orders in disputes * fix: more meaningful variable name in dispute setup * fix: wrong commented lines reactivated * fix: some fixes in privacy orders with encryption * fix: some fixes following coderabbit advices Introduced a random delay on password entering to avoid timing attacks. Removed awaiting from store_encrypted function that does not require async. * refactor: removed many useless connection opening to database with direct use of borrowed pool variable * chore: more meaningful text for user in db.rs * chore: improved encryption unit test now fix salt for testing works - fix salt and password are used to test timings with decryption using key caching, to test it do: cargo test test_fetch_string_column_scalar -- --nocapture * refactor: some improvements in db.rs improved password error management reduce code duplication for pending orders checks * chore(rebase-on-main): commit cargo.lock for rebasing on top of main encrytpion * feat: completing refactor and rebase of encryption pr * refactor: introduced new refactoring of encryption-decryption function - cargo compiles now, we will have to create the new mostro-core after some testings. - bumped nostr-sdk to 0.41 release, no breaking changes * chore: temporary added git branch of mostro-core to compile * chore(test): fix tests * chore: bumped mostro-core version in cargo.toml * fix: wrong database path string creation * chore: fix tests in db.rs * fix: child order after a trade with a full privacy user have wrong master keys * fix: typo on check password hash function * fix: child order event after a complete order does not send user information also if order is normal (not full privacy) * fix: if a child order come from a privacy order user rating are sent in the event wit all zeroes * fix: fixed bug found by Catrya - when I recreate the child order master keys were saved without encryption so when a takesell or takebuy comes in the search for pending user orders generate a decrytpion error. --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Francisco Calderón <fjcalderon@gmail.com>
@grunch @Catrya ,
some fixes on
mostrodto make disputes work again.There were some wrong uses of keys check and some wrong returns in case of
admin-settleandadmin-cancel.I have been able to open a dispute, take as admin and settle it.
Also
mostro-clineeds some changes. You will find a relative pr.Summary by CodeRabbit
Refactor
New Features
LnStatusto ensure its availability in the module.mostro-coredependency to enhance performance and features.