refactor: removed dead code identified in issue - #848
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
Included review availability: Your plan includes up to 2 reviews per rolling hour; 1 remains after this review. WalkthroughThe pull request removes obsolete validation and fallback branches, propagates dispute setup failures, and replaces disputes-table rebuilding with transactional removal of legacy token columns. ChangesApplication and database cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The current changes are merge-ready after normal checks and review; no actionable merge-blocking risk remains. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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.
🧹 Nitpick comments (1)
src/db.rs (1)
3410-3416: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a positive-path migration test.
With
rebuild_disputes_table_preserves_rowsgone, the only remaining coverage is the no-op branch — the actual drop path (and row preservation) is untested. A test that adds the legacy columns back, inserts a row, runs the migration, and asserts both columns are gone while the row survives would keep coverage on the reachable logic this PR is consolidating onto.🧪 Sketch of the missing test
#[tokio::test] async fn migrate_remove_token_columns_drops_legacy_columns_and_preserves_rows() { let pool = migrated_pool().await; sqlx::query("ALTER TABLE disputes ADD COLUMN buyer_token INTEGER") .execute(&pool) .await .unwrap(); sqlx::query("ALTER TABLE disputes ADD COLUMN seller_token INTEGER") .execute(&pool) .await .unwrap(); // insert a dispute row here, then: migrate_remove_token_columns(&pool).await.unwrap(); assert!(!table_column_exists(&pool, "disputes", "buyer_token").await.unwrap()); assert!(!table_column_exists(&pool, "disputes", "seller_token").await.unwrap()); // assert the inserted row is still present }🤖 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/db.rs` around lines 3410 - 3416, Add a positive-path test alongside migrate_remove_token_columns_is_noop_without_token_columns that restores both legacy token columns, inserts a disputes row, runs migrate_remove_token_columns, and verifies buyer_token and seller_token are removed while the inserted row remains.
🤖 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/db.rs`:
- Around line 3410-3416: Add a positive-path test alongside
migrate_remove_token_columns_is_noop_without_token_columns that restores both
legacy token columns, inserts a disputes row, runs migrate_remove_token_columns,
and verifies buyer_token and seller_token are removed while the inserted row
remains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 46a3b8f6-0bd3-4fdf-b711-402423c284ff
📒 Files selected for processing (4)
src/app/admin_take_dispute.rssrc/app/dispute.rssrc/app/restore_session.rssrc/db.rs
💤 Files with no reviewable changes (1)
- src/app/restore_session.rs
ToRyVand
left a comment
ToRyVand
left a comment
There was a problem hiding this comment.
Reviewed the four changes against main. The substance is correct — both dead-code claims hold up under checking — but I found three things worth surfacing before merge.
Verified as genuinely unreachable (both claims correct):
restore_session.rs— the hex/length guards were dead.master_keyandtrade_keycome fromevent.identity/event.sender, which are alreadynostr_sdk::PublicKey;to_string()on those can only ever produce 64-char hex, so neither guard could fire. ThePublicKey::from_hexcalls further down insend_restore_session_response/send_restore_session_timeoutstill validate the string form where it actually matters, and their tests still cover the invalid-key path.admin_take_dispute.rs— theis_solver == 0recheck was structurally unreachable:find_solver_pubkeyqueriesWHERE pubkey == ?1 AND is_solver == trueviafetch_one, so anOkarm necessarily carriesis_solver == true.
Three findings:
1. The branch is 32 commits behind upstream/main and needs a rebase before merge. Side effect while reviewing: the suite here runs 1045 tests vs 1187 on current main, which initially looked like deleted test coverage and isn't — it's just the older base. Worth rebasing so a reviewer doesn't have to rule that out.
2. The SQLite justification reaches the right conclusion from the wrong premise. The commit message argues DROP COLUMN is safe by enumerating distro versions (Ubuntu 24.04 = 3.45, Bookworm = 3.40, Alpine 3.43+). But mostrod never uses the host's SQLite: libsqlite3-sys 0.37.0 compiles the bundled amalgamation (cargo:rerun-if-changed=sqlite3/sqlite3.c) and links it statically (cargo:rustc-link-lib=static=sqlite3), pinning SQLite 3.51.3 at compile time. So removing the fallback is safer than argued — it cannot depend on the deployment environment at all. Only flagging because that reasoning will be the record for whoever revisits this: the distro-version framing implies a host dependency that doesn't exist, and would send someone re-adding a fallback for a machine with old system SQLite.
3. The dispute.rs change is a behavior fix, not dead-code removal — you do say so in the description, so this is context for @grunch rather than a correction. Tracing it: setup_dispute returns Err(CantDoReason::DisputeCreationError) only when the disputing party's flag was already set. Old code skipped order.update() but kept going and still created the dispute row; new code returns early. Returning early is right.
The nuance: dispute_action already guards at the top with find_dispute_by_order_id(...).is_ok() → DisputeAlreadyExists, so the normal double-dispute path never reaches setup_dispute twice. That means the Err branch is only reachable from an inconsistent DB state (order flag set with no dispute row), where the old code silently self-healed and the new code hard-fails with DisputeCreationError — leaving that user unable to open a dispute until the state is fixed. I think failing loudly on a violated precondition is the correct call, and it's a state that shouldn't arise. Just worth a maintainer knowing it's the trade-off being made rather than discovering it from a report later.
Checks on the branch as-is: 1045 passed / 0 failed, cargo clippy --all-targets -- -D warnings clean, cargo fmt --check clean. The three migrate_remove_token_columns tests (no-op, both-columns, single-column) survive the rewrite, so the migration keeps its coverage.
Nothing here is a blocker on the code itself — my only actual ask is the rebase, plus optionally correcting the SQLite rationale so it doesn't mislead later. Contributor, not a maintainer, so this is a technical second opinion rather than a merge signal.
- restore_session: drop unreachable hex-validity guards on PublicKey values; nostr_sdk::PublicKey always serializes to 64-char hex so the guards could never trigger - admin_take_dispute: drop is_solver == 0 recheck after find_solver_pubkey; the SQL already filters WHERE is_solver == true so the Ok arm is structurally guaranteed non-zero - db: remove rebuild_disputes_table_without_tokens and the SQLite version-fallback branch inside migrate_remove_token_columns; DROP COLUMN is safe unconditionally - mostrod links libsqlite3-sys with \ the bundled SQLite source (3.51.3 compiled statically into the binary),\ so runtime behavior is independent of any host SQLite version - dispute: propagate setup_dispute error with map_err(MostroCantDo)? instead of silently swallowing it; the old if .is_ok() pattern skipped order.update() on failure but kept running, leaving the order flags unset while still creating the dispute row
9fc0299 to
69542b3
Compare
|
Thank you for the feedback @ToRyVand Also corrected the SQLite rationale in the commit message. The distro-version framing was wrong — mostrod links libsqlite3-sys 0.37.0 which compiles and statically links the bundled SQLite source (3.51.3) at build time, so the fallback removal is independent of any host SQLite version. Updated the message to reflect that. |
|
Verified independently against One note: rebase was onto |
cargo mutants left the `60 * 60` timeout computation untestable inline. Extract it as RESTORE_SESSION_TIMEOUT_SECS with a test that pins the value. The hex-validation extraction this commit originally carried is dropped: its only two call sites are the guards MostroP2P#848 removes as unreachable (`identity`/`sender` are `PublicKey`, so `.to_string()` is always 64 hex), and the two invalid-key tests it added already exist on main from MostroP2P#803.
2 participants
restore_session: drop unreachable hex-validity guards on PublicKey values; nostr_sdk::PublicKey always serializes to 64-char hex so the guards could never trigger
admin_take_dispute: drop is_solver == 0 recheck after find_solver_pubkey; the SQL already filters WHERE is_solver == true so the Ok arm is structurally guaranteed non-zero
db: remove rebuild_disputes_table_without_tokens and the SQLite version-fallback branch inside migrate_remove_token_columns; DROP COLUMN is supported on all deployment targets (SQLite >= 3.35 everywhere: Ubuntu 24.04 = 3.45, Debian Bookworm = 3.40, Alpine = 3.43+)
dispute: propagate setup_dispute error with map_err(MostroCantDo)? instead of silently swallowing it; the old if .is_ok() pattern skipped order.update() on failure but kept running, leaving the order flags unset while still creating the dispute row
Closes #818
Summary by CodeRabbit