Skip to content

refactor: removed dead code identified in issue #848

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
grunch merged 1 commit into from
Aug 20, 2026
+56 −220
Merged

refactor: removed dead code identified in issue #848

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/app/admin_take_dispute.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,10 +105,10 @@ pub async fn pubkey_event_can_solve(
}

// Sender must be a solver user
let Ok(solver) = find_solver_pubkey(pool, sender_pubkey.clone()).await else {
return false;
};
if solver.is_solver == 0_i64 {
if find_solver_pubkey(pool, sender_pubkey.clone())
.await
.is_err()
{
return false;
}

Expand Down
15 changes: 8 additions & 7 deletions src/app/dispute.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,13 +169,14 @@ pub async fn dispute_action(
let dispute = Dispute::new(order_id, order.status.clone());

// Setup dispute
if order.setup_dispute(is_buyer_dispute).is_ok() {
order
.clone()
.update(pool)
.await
.map_err(|cause| MostroInternalErr(ServiceError::DbAccessError(cause.to_string())))?;
}
order

@grunch grunch Aug 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOW | test coverage

The new error path has no test. grep -rn DisputeCreationError src/ returns nothing.

This change turns a silently swallowed error into a user-visible MostroCantDo(CantDoReason::DisputeCreationError) response — a behavior change, as your description says — and nothing exercises it. The existing dispute_action_* tests cover NotFound, already-exists, non-disputable status, missing seller/buyer pubkey, non-party sender, and both happy paths; this new arm is the one gap.

It's reachable in a test by inserting an order with buyer_dispute = 1 and no row in disputes, then asserting dispute_action returns MostroCantDo(CantDoReason::DisputeCreationError) and that no dispute row was created — which also pins the intended "fail loudly on inconsistent state" semantics that @ToRyVand flagged as the trade-off being made here.

.setup_dispute(is_buyer_dispute)
.map_err(MostroCantDo)?;

@grunch grunch Aug 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT | note for future readers

In mostro-core 0.14.5 (order.rs:537) setup_dispute assigns self.status = Status::Dispute.to_string() before the DisputeCreationError return, so on Err the local order is left dirty.

The new code returns immediately and never persists it, so this is strictly safer than the old if .is_ok() form. But anyone who later moves that ? or reuses order after this point inherits a trap. A one-line comment would make the invariant explicit:

// setup_dispute leaves order.status dirty on Err; we return before any persist.
order.setup_dispute(is_buyer_dispute).map_err(MostroCantDo)?;

order
.clone()
.update(pool)
.await
.map_err(|cause| MostroInternalErr(ServiceError::DbAccessError(cause.to_string())))?;

// Save dispute to database
let dispute = dispute
Expand Down
10 changes: 0 additions & 10 deletions src/app/restore_session.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,6 @@ pub async fn restore_session_action(
// Get trade key from the event rumor
let trade_key = event.sender.to_string();

// Validate the master key format
if !master_key.chars().all(|c| c.is_ascii_hexdigit()) || master_key.len() != 64 {
return Err(MostroCantDo(CantDoReason::InvalidPubkey));
}

// Validate the trade key format
if !trade_key.chars().all(|c| c.is_ascii_hexdigit()) || trade_key.len() != 64 {
return Err(MostroCantDo(CantDoReason::InvalidPubkey));
}

// No key in the log line — master_key is the user's persistent Nostr
// identity, not a per-trade ephemeral one (AGENTS.md: scrub Nostr keys
// from logs).
Expand Down
243 changes: 44 additions & 199 deletions src/db.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,205 +79,63 @@ pub async fn find_active_trade_pubkeys(pool: &SqlitePool) -> Result<Vec<String>,
Ok(keys.into_iter().collect())
}

/// Helper function to rebuild disputes table without token columns when DROP COLUMN is unsupported.
async fn rebuild_disputes_table_without_tokens(pool: &SqlitePool) -> Result<(), MostroError> {
tracing::info!("Rebuilding disputes table without token columns (SQLite compatibility mode)");

// Create temporary table with new schema (without token columns)
sqlx::query(
r#"
CREATE TABLE IF NOT EXISTS disputes_temp (
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
.map_err(|e| {
MostroInternalErr(ServiceError::DbAccessError(format!(
"Failed to create temporary disputes table: {}",
e
)))
})?;

// Copy data from original table to temporary table (excluding token columns)
sqlx::query(
r#"
INSERT INTO disputes_temp (id, order_id, status, order_previous_status, solver_pubkey, created_at, taken_at)
SELECT id, order_id, status, order_previous_status, solver_pubkey, created_at, taken_at
FROM disputes
"#,
)
.execute(pool)
.await
.map_err(|e| MostroInternalErr(ServiceError::DbAccessError(format!(
"Failed to copy data to temporary table: {}", e
))))?;

// Drop original table
sqlx::query("DROP TABLE disputes")
.execute(pool)
.await
.map_err(|e| {
MostroInternalErr(ServiceError::DbAccessError(format!(
"Failed to drop original disputes table: {}",
e
)))
})?;

// Rename temporary table to disputes
sqlx::query("ALTER TABLE disputes_temp RENAME TO disputes")
.execute(pool)
.await
.map_err(|e| {
MostroInternalErr(ServiceError::DbAccessError(format!(
"Failed to rename temporary table: {}",
e
)))
})?;

tracing::info!("Successfully rebuilt disputes table without token columns");
Ok(())
}

/// Migrates legacy disputes table by removing deprecated buyer_token and seller_token columns if present.
/// Removes deprecated `buyer_token` and `seller_token` columns from the disputes table if present.

@grunch grunch Aug 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT | documentation accuracy

"Both drops run inside a single transaction so either both succeed or neither does" is accurate for the two ALTER TABLE statements, but the table_column_exists calls that decide what to drop run on a separate pooled connection, before pool.begin().

Harmless in practice — single caller, at startup, before the daemon serves anything — and unchanged from the previous implementation. Just worth not implying the whole read-decide-write sequence is atomic, since a future reader may rely on that claim.

///
/// This function uses transactions for atomic operations and includes fallback logic for older SQLite versions
/// that don't support ALTER TABLE DROP COLUMN. The function handles both cases where columns exist (legacy databases)
/// and don't exist (newer databases).
/// Both drops run inside a single transaction so either both succeed or neither does.
/// This is a no-op when neither column exists (fresh installs and already-migrated databases).
async fn migrate_remove_token_columns(pool: &SqlitePool) -> Result<(), MostroError> {

@grunch grunch Aug 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MEDIUM | follow-up, not introduced by this PR

The drop path this PR keeps is itself unreachable in production.

migrations/20230928145530_disputes.sql was edited in place by #516 (ab338c5) to remove the two token columns. sqlx 0.9 Migrator::run calls validate_applied_migrations (sqlx-core-0.9.0/src/migrate/migrator.rs:255,274) and returns MigrateError::VersionMismatch whenever a recorded checksum differs from the file on disk.

So any database old enough to still carry buyer_token/seller_token fails at migrator.run(&conn) inside connect() — and connect() only recovers from duplicate column name errors via parse_duplicate_column_name / reconcile_existing_add_column_migration, never from VersionMismatch. migrate_remove_token_columns is therefore never reached on exactly the databases it exists to fix.

This PR is trimming ~140 lines from a function whose one remaining non-trivial branch is dead for the same class of reason as the branches #818 asked you to delete. Not a blocker and out of scope here, but worth an issue: either delete migrate_remove_token_columns and its tests entirely, or extend the existing reconciliation to handle VersionMismatch if pre-#516 databases are genuinely meant to be supported.

// Check if token columns exist
let buyer_token_exists = sqlx::query_scalar::<_, i32>(
r#"
SELECT COUNT(*)
FROM pragma_table_info('disputes')
WHERE name = 'buyer_token'
"#,
)
.fetch_one(pool)
.await
.map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?
> 0;

let seller_token_exists = sqlx::query_scalar::<_, i32>(
r#"
SELECT COUNT(*)
FROM pragma_table_info('disputes')
WHERE name = 'seller_token'
"#,
)
.fetch_one(pool)
.await
.map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?
> 0;
let buyer_token_exists = table_column_exists(pool, "disputes", "buyer_token").await?;
let seller_token_exists = table_column_exists(pool, "disputes", "seller_token").await?;

// If no token columns exist, no migration needed
if !buyer_token_exists && !seller_token_exists {
tracing::debug!(
"No deprecated token columns found in disputes table - migration not needed"
);
return Ok(());
}

// Check SQLite version to determine if DROP COLUMN is supported
let sqlite_version = sqlx::query_scalar::<_, String>("SELECT sqlite_version()")
.fetch_one(pool)
.await
.map_err(|e| {
MostroInternalErr(ServiceError::DbAccessError(format!(
"Failed to get SQLite version: {}",
e
)))
})?;

tracing::info!("SQLite version: {}", sqlite_version);

// Parse version to check if DROP COLUMN is supported (requires 3.35.0+)
let supports_drop_column = sqlite_version
.split('.')
.take(3)
.map(|v| v.parse::<u32>().unwrap_or(0))
.collect::<Vec<_>>()
.get(..3)
.map(|parts| {
let major = parts[0];
let minor = parts.get(1).copied().unwrap_or(0);
major > 3 || (major == 3 && minor >= 35)
})
.unwrap_or(false);

if supports_drop_column {
// Try DROP COLUMN approach with transaction
tracing::info!(
"Attempting to remove token columns using DROP COLUMN (SQLite {})...",
sqlite_version
);

let mut transaction = pool.begin().await.map_err(|e| {
MostroInternalErr(ServiceError::DbAccessError(format!(
"Failed to begin transaction: {}",
e
)))
})?;

// Attempt to drop columns within transaction
let drop_result = async {
if buyer_token_exists {
sqlx::query("ALTER TABLE disputes DROP COLUMN buyer_token")
.execute(&mut *transaction)
.await?;
tracing::info!("Dropped buyer_token column");
}
let mut tx = pool.begin().await.map_err(|e| {
MostroInternalErr(ServiceError::DbAccessError(format!(
"Failed to begin transaction: {}",
e
)))
})?;

if seller_token_exists {
sqlx::query("ALTER TABLE disputes DROP COLUMN seller_token")
.execute(&mut *transaction)
.await?;
tracing::info!("Dropped seller_token column");
}
if buyer_token_exists {
sqlx::query("ALTER TABLE disputes DROP COLUMN buyer_token")
.execute(&mut *tx)
.await
.map_err(|e| {
MostroInternalErr(ServiceError::DbAccessError(format!(
"Failed to drop buyer_token column: {}",
e
)))
})?;
tracing::info!("Dropped buyer_token column from disputes table");
}

if seller_token_exists {
sqlx::query("ALTER TABLE disputes DROP COLUMN seller_token")
.execute(&mut *tx)
.await
.map_err(|e| {
MostroInternalErr(ServiceError::DbAccessError(format!(
"Failed to drop seller_token column: {}",
e
)))
})?;
tracing::info!("Dropped seller_token column from disputes table");
}

Ok::<(), sqlx::Error>(())
}
.await;
tx.commit().await.map_err(|e| {
MostroInternalErr(ServiceError::DbAccessError(format!(
"Failed to commit transaction: {}",
e
)))
})?;

match drop_result {
Ok(_) => {
transaction.commit().await.map_err(|e| {
MostroInternalErr(ServiceError::DbAccessError(format!(
"Failed to commit transaction: {}",
e
)))
})?;
tracing::info!("Successfully removed token columns using DROP COLUMN");
Ok(())
}
Err(e) => {
tracing::warn!("DROP COLUMN failed ({}), falling back to table rebuild", e);
transaction.rollback().await.map_err(|rollback_err| {
MostroInternalErr(ServiceError::DbAccessError(format!(
"Failed to rollback transaction: {}",
rollback_err
)))
})?;

// Fall back to table rebuild
rebuild_disputes_table_without_tokens(pool).await
}
}
} else {
// SQLite version doesn't support DROP COLUMN, use table rebuild
tracing::info!(
"SQLite version {} doesn't support DROP COLUMN, using table rebuild method",
sqlite_version
);
rebuild_disputes_table_without_tokens(pool).await
}
tracing::info!("Successfully removed deprecated token columns from disputes table");
Ok(())
}

async fn table_column_exists(
Expand Down Expand Up @@ -4973,19 +4831,6 @@ mod migration_and_query_tests {
.unwrap());
}

#[tokio::test]
async fn rebuild_disputes_table_preserves_rows() {
let pool = migrated_pool().await;
let order_id = Uuid::new_v4();
insert_dispute(&pool, order_id, "in-progress", Some(HEX_KEY_B)).await;

rebuild_disputes_table_without_tokens(&pool).await.unwrap();

let dispute = find_dispute_by_order_id(&pool, order_id).await.unwrap();
assert_eq!(dispute.order_id, order_id);
assert_eq!(dispute.status, "in-progress");
}

// ── admin / permission queries ───────────────────────────────────────

#[tokio::test]
Expand Down