Improve error message when admin takes over dispute from solver#519
Conversation
Otso-Hirvikangas
force-pushed
the
feat/issue-302-admin-dispute-takeover-message
branch
from
0c390d6 to
12611dd
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughUpdate mostro-core dependency version in Cargo.toml; add a new DB helper Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Solver
participant App as Admin Cancel/Settle Handler
participant DB
Solver->>App: request cancel/settle(order_id)
App->>DB: is_assigned_solver(order_id, solver_pubkey)
alt assigned to solver
App-->>Solver: proceed (existing success flow)
else not assigned
App->>DB: is_dispute_taken_by_admin(order_id)
alt admin took dispute
App-->>Solver: Error: DisputeTakenByAdmin
else admin did not take
App-->>Solver: Error: IsNotYourDispute
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
Cargo.toml (1)
67-67: Pin the git dependency to a commit (and plan revert to crates.io).Floating at repo HEAD is non-reproducible and risky for releases. Pin to a specific commit and switch back to a version (e.g., >= 0.6.51) once published.
Apply this diff, replacing <COMMIT_SHA> with the merge commit that introduced DisputeTakenByAdmin in mostro-core:
-mostro-core = { git = "https://github.com/MostroP2P/mostro-core.git", features = ["sqlx"] } +mostro-core = { git = "https://github.com/MostroP2P/mostro-core.git", rev = "<COMMIT_SHA>", features = ["sqlx"] }Optionally set
[package.metadata.release].publish = falseuntil the crate version is published to avoidcargo publishfailures with git deps.src/db.rs (1)
1154-1180: Avoid magic string for status and propagate key retrieval errors (safer, more robust).Use DisputeStatus::InProgress in the query to prevent drift, and map
get_keys()errors instead of silently returning false.Apply this diff:
/// Check if a dispute has been taken over by admin (Mostro daemon) /// This helps provide better error messages when solver tries to act on admin-taken disputes pub async fn is_dispute_taken_by_admin( pool: &SqlitePool, order_id: Uuid, ) -> Result<bool, MostroError> { - // Get the dispute for this order - let dispute = sqlx::query( - "SELECT solver_pubkey FROM disputes WHERE order_id = ? AND status = 'in-progress'", - ) - .bind(order_id) + // Get the dispute for this order (only active one) + let dispute = sqlx::query( + "SELECT solver_pubkey FROM disputes WHERE order_id = ? AND status = ?", + ) + .bind(order_id) + .bind(DisputeStatus::InProgress.to_string()) .fetch_optional(pool) .await .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; if let Some(row) = dispute { - if let Some(solver_pubkey) = row.try_get::<Option<String>, _>("solver_pubkey") - .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))? { - // Check if the current solver is the admin (mostro daemon) - if let Ok(my_keys) = crate::util::get_keys() { - return Ok(solver_pubkey == my_keys.public_key().to_string()); - } - } + let solver_pubkey: Option<String> = row + .try_get("solver_pubkey") + .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; + if let Some(solver_pubkey) = solver_pubkey { + // Check if the current solver is the admin (mostro daemon) + let my_keys = crate::util::get_keys() + .map_err(|e| MostroInternalErr(ServiceError::NostrError(e.to_string())))?; + return Ok(solver_pubkey == my_keys.public_key().to_string()); + } } Ok(false) }src/app/admin_cancel.rs (1)
29-34: Good: clearer error for admin takeover.Branching to DisputeTakenByAdmin vs IsNotYourDispute improves UX as intended.
This solver-ownership check now duplicates in admin_settle.rs. Consider extracting a small helper (e.g.,
cant_do_for_solver(pool, sender, order_id) -> MostroError) to keep the logic in one place.src/app/admin_settle.rs (1)
162-185: Make the test resilient: avoid asserting on Debug strings.Comparing
{:?}output is brittle. Match on variants instead.Apply this diff:
#[cfg(test)] mod tests { use super::*; use mostro_core::error::CantDoReason; /// Test that our error handling logic correctly identifies admin takeover vs regular disputes /// This tests the core business logic of issue #302 without complex database setup #[test] fn test_dispute_error_types() { - // Test that we have the correct error types available - // This ensures our mostro-core dependency includes the new DisputeTakenByAdmin variant - - // Original error for regular dispute issues - let regular_error = CantDoReason::IsNotYourDispute; - assert_eq!(format!("{:?}", regular_error), "IsNotYourDispute"); - - // New error for admin takeover scenarios - let admin_error = CantDoReason::DisputeTakenByAdmin; - assert_eq!(format!("{:?}", admin_error), "DisputeTakenByAdmin"); - - // Verify they are different error types - assert_ne!(regular_error, admin_error); + let regular_error = CantDoReason::IsNotYourDispute; + let admin_error = CantDoReason::DisputeTakenByAdmin; + assert!(matches!(regular_error, CantDoReason::IsNotYourDispute)); + assert!(matches!(admin_error, CantDoReason::DisputeTakenByAdmin)); + assert_ne!(regular_error, admin_error); // sanity: distinct variants } }
📜 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 (4)
Cargo.toml(1 hunks)src/app/admin_cancel.rs(2 hunks)src/app/admin_settle.rs(3 hunks)src/db.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-25T22:48:44.741Z
Learnt from: Catrya
PR: MostroP2P/mostro#502
File: src/app/release.rs:52-55
Timestamp: 2025-06-25T22:48:44.741Z
Learning: Cross-repository dependencies in Mostro project: when PRs introduce new types like PaymentFailedInfo from mostro-core, compilation errors are expected until the dependency PR is merged and the version is updated in the consuming repository.
Applied to files:
Cargo.toml
🔇 Additional comments (2)
src/app/admin_settle.rs (2)
1-1: Import looks correct and scoped.The added
is_dispute_taken_by_adminimport is appropriate.
32-41: Good: map solver-mismatch to DisputeTakenByAdmin when applicable.Matches the PR objective and preserves previous behavior otherwise.
Please confirm
CantDoReason::DisputeTakenByAdminis available from the pinned mostro-core commit; otherwise, builds on fresh clones may fail until the crate release is cut.
Catrya
left a comment
Catrya
left a comment
There was a problem hiding this comment.
tACK , thanks @Otso-Hirvikangas
There was a problem hiding this comment.
great job @Otso-Hirvikangas, please update mostro core version to v0.6.51
Replace generic "Dispute not taken by you" with specific "DisputeTakenByAdmin" error when a solver tries to resolve a dispute that an admin has taken over. - Add is_dispute_taken_by_admin() function to check if dispute solver is admin - Update admin_cancel.rs and admin_settle.rs to use new error type - Include unit test for error type accessibility - Update mostro-core dependency to v0.6.51 (published version with DisputeTakenByAdmin) Fixes MostroP2P#302
Otso-Hirvikangas
force-pushed
the
feat/issue-302-admin-dispute-takeover-message
branch
from
f4b46d7 to
577aa3b
Compare
|
Thanks for the feedback! mostro-core version updated. |
Otso-Hirvikangas
deleted the
feat/issue-302-admin-dispute-takeover-message
branch
4 participants
Summary
Improve error messaging when admin takes over dispute from solver. Replace generic "Dispute not taken by you" with specific "DisputeTakenByAdmin" error.
Implementation
Adds
is_dispute_taken_by_admin()function that checks if the current dispute solver is the admin (mostro daemon). When a solver tries to resolve a dispute that an admin has taken over, they now get a clear message explaining the situation instead of a generic error.Tests
Includes unit test validating the new error types are accessible and distinct from existing ones.
Dependencies
mostro-core = { git = "https://github.com/MostroP2P/mostro-core.git" }Status: Ready for review
Fixes #302
Summary by CodeRabbit
New Features
Chores
Tests