Test/solver write permission gate 709

[codaMW]

Summary

Closes #709.

Adds four behavioral tests covering the read-only rejection and read-write success paths in admin_settle_action and admin_cancel_action, as requested by @grunch during review of PR #708.

Approach

Both action handlers call ensure_dispute_finalize_permission before any Lightning operations. The tests exercise this gate directly rather than calling the full action handler, which would require a mock LND connector. This covers the exact authorization boundary described in the issue while keeping the fixtures minimal and deterministic.

Each test seeds an in-memory SQLite database with the real migration schema, inserts a solver and an assigned dispute, then asserts the permission gate returns the correct result.

Tests added

src/app/admin_settle.rs

• admin_settle_read_only_solver_is_rejected : solver with category=1 receives MostroCantDo(NotAuthorized)
• admin_settle_read_write_solver_is_allowed : solver with category=2 passes the gate

src/app/admin_cancel.rs

• admin_cancel_read_only_solver_is_rejected : solver with category=1 receives MostroCantDo(NotAuthorized)
• admin_cancel_read_write_solver_is_allowed : solver with category=2 passes the gate

Test results

test app::admin_settle::tests::admin_settle_read_only_solver_is_rejected ... ok
test app::admin_settle::tests::admin_settle_read_write_solver_is_allowed ... ok
test app::admin_cancel::tests::admin_cancel_read_only_solver_is_rejected ... ok
test app::admin_cancel::tests::admin_cancel_read_write_solver_is_allowed ... ok

cargo fmt, cargo clippy, and cargo test all green.

Summary by CodeRabbit

• Tests

  • Added and expanded SQLite-backed async tests covering permission checks for dispute actions, validating both read-only and read-write solver access and related order scenarios.

• Bug Fixes

  • Improved error handling in dispute and order flows: missing orders or missing order identifiers now return explicit "Not Found", while actual database access failures are reported as internal errors.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0cc1e9a2-81b0-47af-9731-10ffe9849f16

📥 Commits

Reviewing files that changed from the base of the PR and between 55b2a21 and 8a39743.

📒 Files selected for processing (2)

• src/app/admin_cancel.rs
• src/app/admin_settle.rs

🚧 Files skipped from review as they are similar to previous changes (2)

• src/app/admin_cancel.rs
• src/app/admin_settle.rs

---

Walkthrough

Standardizes order-not-found errors to return MostroCantDo(CantDoReason::NotFound) and adds SQLite-backed Tokio tests validating that read-only solvers are rejected and read-write solvers are allowed by the dispute finalize permission gate.

Changes

Error Handling and Permission Gate Tests

Layer / File(s)	Summary
Order lookup error normalization src/util.rs, src/app/dispute.rs, src/app/admin_take_dispute.rs	get_order and callers now return MostroCantDo(CantDoReason::NotFound) when order id is missing or the order isn't found. Added three Tokio tests covering missing id, absent DB row, and successful retrieval.
Dispute finalize permission gate tests src/app/admin_settle.rs, src/app/admin_cancel.rs	Adds in-memory SQLite test helpers and Tokio tests that seed solver fixtures (read-only vs read-write) and disputes, asserting ensure_dispute_finalize_permission rejects read-only solvers with CantDoReason::NotAuthorized and allows read-write solvers.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• MostroP2P/mostro#708: Implements the read vs read-write solver authorization logic these tests exercise.
• MostroP2P/mostro#746: Introduced/used ensure_dispute_finalize_permission, which these tests validate end-to-end.

Suggested reviewers

• arkanoider
• grunch

Poem

🐰 I hopped through rows of orders late at night,
I nudged the NotFound into the light,
Read-only gates said “no,” write keys said “go,”
Tests now prove the path, all tidy and bright.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	Changes to admin_take_dispute.rs, dispute.rs, and util.rs introducing error handling changes (InvalidOrderId → NotFound) are out of scope for issue #709 which focuses solely on permission gate testing.	Remove or isolate error handling refactors to a separate PR; keep only test additions for admin_settle and admin_cancel permission gates in this changeset.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	Title clearly identifies the PR is about testing the solver write permission gate for issue #709, directly matching the changeset's focus.
Linked Issues check	✅ Passed	All acceptance criteria from #709 are met: tests for both admin_settle and admin_cancel covering read-only rejection and read-write success paths using in-memory SQLite with proper seeding.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (2)

src/app/admin_settle.rs (1)

263-297: ⚡ Quick win

Consider using the actual disputes migration instead of manual DDL.

The helper manually creates the disputes table with a CREATE TABLE statement, while the other tables use migration files via include_str!. If a disputes migration exists in migrations/, including it would ensure the test schema stays in sync with production.

♻️ Recommended approach

Check if a migration file like migrations/*_disputes.sql exists. If so, replace lines 282–295 with:

-        sqlx::query(
-            r#"CREATE TABLE IF NOT EXISTS disputes (
-                id char(36) primary key not null,
-                order_id char(36) unique not null,
-                status varchar(10) not null,
-                order_previous_status varchar(10) not null,
-                solver_pubkey char(64),
-                created_at integer not null,
-                taken_at integer default 0
-            )"#,
-        )
-        .execute(&pool)
-        .await
-        .unwrap();
+        sqlx::query(include_str!("../../migrations/YYYYMMDDHHMMSS_disputes.sql"))
+            .execute(&pool)
+            .await
+            .unwrap();

Replace YYYYMMDDHHMMSS with the actual migration timestamp.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/app/admin_settle.rs` around lines 263 - 297, The setup_permission_db
helper currently creates the disputes table via manual DDL inside async fn
setup_permission_db(); replace that manual CREATE TABLE block with executing the
actual disputes migration file (use
sqlx::query(include_str!("../../migrations/YYYYMMDDHHMMSS_disputes.sql")).execute(&pool).await.unwrap())
so the test DB schema stays in sync with production; locate the CREATE TABLE for
"disputes" in setup_permission_db and swap it to load the corresponding
migration file (use the real migration timestamp in the filename).

src/app/admin_cancel.rs (1)

244-289: ⚖️ Poor tradeoff

Consider extracting shared test helpers to reduce duplication.

The test constants, setup_permission_db, and seed helpers (lines 244–313) are duplicated between admin_settle.rs and admin_cancel.rs. Extracting them into a shared test module (e.g., src/app/test_helpers.rs or src/app/admin_test_helpers.rs) would reduce maintenance burden and ensure schema consistency across test suites.

♻️ Optional refactor approach

Create src/app/dispute_test_helpers.rs:

#[cfg(test)]
pub(crate) mod dispute_test_helpers {
    pub const DAEMON_PUBKEY: &str = "b1b2c3d4...";
    pub const READ_ONLY_SOLVER: &str = "c1b2c3d4...";
    pub const READ_WRITE_SOLVER: &str = "d1b2c3d4...";

    pub async fn setup_permission_db() -> sqlx::SqlitePool {
        // ... shared implementation
    }

    pub async fn seed_solver(pool: &sqlx::SqlitePool, pubkey: &str, category: i64) {
        // ... shared implementation
    }

    pub async fn seed_dispute(pool: &sqlx::SqlitePool, order_id: uuid::Uuid, solver_pubkey: &str) {
        // ... shared implementation
    }
}

Then import in both test modules:

#[cfg(test)]
mod tests {
    use super::super::dispute_test_helpers::*;
    // ... tests
}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/app/admin_cancel.rs` around lines 244 - 289, The tests duplicate
constants DAEMON_PUBKEY, READ_ONLY_SOLVER, READ_WRITE_SOLVER and the async
helper setup_permission_db; extract these into a shared test helper module
(e.g., create src/app/dispute_test_helpers.rs with #[cfg(test)] pub(crate) mod
dispute_test_helpers) that exposes the three consts and a pub async fn
setup_permission_db() plus any seed helpers, then update the tests in
admin_cancel.rs and admin_settle.rs to use dispute_test_helpers::{DAEMON_PUBKEY,
READ_ONLY_SOLVER, READ_WRITE_SOLVER, setup_permission_db} to remove duplication
and keep schema/seed logic centralized.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/app/admin_cancel.rs`:
- Around line 244-289: The tests duplicate constants DAEMON_PUBKEY,
READ_ONLY_SOLVER, READ_WRITE_SOLVER and the async helper setup_permission_db;
extract these into a shared test helper module (e.g., create
src/app/dispute_test_helpers.rs with #[cfg(test)] pub(crate) mod
dispute_test_helpers) that exposes the three consts and a pub async fn
setup_permission_db() plus any seed helpers, then update the tests in
admin_cancel.rs and admin_settle.rs to use dispute_test_helpers::{DAEMON_PUBKEY,
READ_ONLY_SOLVER, READ_WRITE_SOLVER, setup_permission_db} to remove duplication
and keep schema/seed logic centralized.

In `@src/app/admin_settle.rs`:
- Around line 263-297: The setup_permission_db helper currently creates the
disputes table via manual DDL inside async fn setup_permission_db(); replace
that manual CREATE TABLE block with executing the actual disputes migration file
(use
sqlx::query(include_str!("../../migrations/YYYYMMDDHHMMSS_disputes.sql")).execute(&pool).await.unwrap())
so the test DB schema stays in sync with production; locate the CREATE TABLE for
"disputes" in setup_permission_db and swap it to load the corresponding
migration file (use the real migration timestamp in the filename).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ded35d11-72fc-44b9-8636-65584ef3c852

📥 Commits

Reviewing files that changed from the base of the PR and between bab00e4 and 99426f8.

📒 Files selected for processing (5)

• src/app/admin_cancel.rs
• src/app/admin_settle.rs
• src/app/admin_take_dispute.rs
• src/app/dispute.rs
• src/util.rs

[arkanoider]

Maybe we can reuse this shorter form for setting up migrations in all files to complete PR:

    async fn create_test_pool() -> SqlitePool {
        let pool = SqlitePool::connect("sqlite::memory:").await.unwrap();
        sqlx::migrate!().run(&pool).await.unwrap();
        pool
    }

The logic of test is ok!

[codaMW]

Thanks @arkanoider for the insight, done in 8a39743. Replaced the manual four-file migration sequence with
sqlx::migrate!() in both admin_settle.rs and admin_cancel.rs.
All 13 admin tests still pass.

[arkanoider]

tACK for me