chore: upgrade sqlx to 0.9 and drop sqlx-crud#791
Conversation
Replace the unmaintained sqlx-crud dependency with the in-tree Crud trait from mostro-core 0.13.2. Add hand-written Bond persistence in bond/crud.rs and switch all Order, Dispute, and Bond call sites to mostro_core::db::Crud. Co-authored-by: Cursor <cursoragent@cursor.com>
Bump mostro-core to 0.14, sqlx 0.9, and MSRV 1.94; replace query! macros with runtime SQL; remove init_db.sh and offline cache files. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughUpdates the project to Rust 1.94.0 and sqlx 0.9.0, removes offline SQLx workflows, and shifts database access toward runtime SQL with ChangesSQLx 0.9 Migration Refresh
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
Actionable comments posted: 2
🤖 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.
Inline comments:
In @.github/workflows/ci.yml:
- Line 69: The checkout step in the CI job is keeping GitHub credentials
persisted in git config unnecessarily. Update the workflow’s actions/checkout
usage to disable persisted credentials by setting persist-credentials to false,
and only keep them enabled if a later step in this same job actually needs to
push changes. Locate the change in the checkout action entry within the CI
workflow.
- Around line 69-70: Both actions in the CI workflow are still referenced by
mutable version tags, so update the `uses:` entries in the MSRV job to immutable
commit SHAs instead of `actions/checkout@v6` and
`dtolnay/rust-toolchain@stable`. Keep the same workflow behavior, but pin these
dependencies to specific revisions so the `ci.yml` job no longer relies on
drifting upstream tags.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 61e3b7f5-2a20-4434-8207-f79f7ae968e3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (35)
.github/workflows/ci.ymlAGENTS.mdCargo.tomlINSTALL.mdREADME.mddocs/ANTI_ABUSE_BOND.mddocs/ARCHITECTURE.mddocs/ORDERS_AND_ACTIONS.mddocs/README.mddocs/STARTUP_AND_CONFIG.mdinit_db.shrust-toolchain.tomlsqlx-data.jsonsrc/app/add_invoice.rssrc/app/admin_cancel.rssrc/app/admin_settle.rssrc/app/admin_take_dispute.rssrc/app/bond/crud.rssrc/app/bond/db.rssrc/app/bond/flow.rssrc/app/bond/mod.rssrc/app/bond/model.rssrc/app/bond/payout.rssrc/app/bond/slash.rssrc/app/cancel.rssrc/app/dispute.rssrc/app/fiat_sent.rssrc/app/rate_user.rssrc/app/release.rssrc/app/take_sell.rssrc/app/trade_pubkey.rssrc/db.rssrc/flow.rssrc/scheduler.rssrc/util.rs
💤 Files with no reviewable changes (2)
- init_db.sh
- sqlx-data.json
Keep sqlx 0.9 / mostro-core 0.14 changes on top of merged #789. Co-authored-by: Cursor <cursoragent@cursor.com>
![ermeme[bot]](https://avatars.githubusercontent.com/in/3690068?s=60&v=4){kind=small}
There was a problem hiding this comment.
Strict review
Request changes: the CI workflow still uses mutable action refs and keeps checkout credentials persisted unnecessarily. Please pin actions/checkout and dtolnay/rust-toolchain to immutable SHAs, and set persist-credentials: false unless a later step in this job actually needs to push.
|
I did a strict pass on this PR and I do not currently see a correctness blocker. What I specifically checked:
My read is that this is a reasonable daemon-side follow-up to the earlier The important part for me was making sure the PR did not reintroduce the unsafe string-dispatch pattern we already pushed back on elsewhere, and it does not. The places that moved away from I also ran So from my side, although this does trade away compile-time SQL checking for the last few |
Delegate find_bond_by_id to Crud::by_id, omit created_at from UPDATE, and add sentinel-based INSERT column/bind alignment tests. Closes #790 Co-authored-by: Cursor <cursoragent@cursor.com>
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
Tested on regtest with mostro-core 0.14.0 from crates.io. ACK, verified the specific hardening claims. Scope (honest): verified build/lint/suite, the three hardening fixes via their specific tests + code paths, migration-on-connect from a fresh DB, and live bond INSERT through the daemon. Did not exercise a full multi-party dispute → slash → payout flow (needs funded LN channels + admin resolution); the bond CRUD paths it would exercise are covered by the unit tests above. |
codaMW
left a comment
codaMW
left a comment
There was a problem hiding this comment.
Tested on regtest with mostro-core 0.14.0 from crates.io. ACK, verified the specific hardening claims.
Build + lint + suite
• Builds clean on Rust 1.94.0 (toolchain auto-synced from rust-toolchain. toml, no friction) against mostro-core 0.14.0 from crates.io.
• cargo clippy --all-targets --all-features -- -D warnings: clean.
• cargo test: 497 passes. The one failure is test_lnurl_validation_with_test_server (AddrInUse) a pre-existing port-bind flake in the Lightning subsystem this PR doesn't touch; fails identically on main, not from this PR.
#790 hardening tasks, each verified individually
• created_at immutability (LOW): push_bond_update_set omits created_at (insert-only, with an explanatory comment); update_preserves_created_at passes.
• find_bond_by_id delegation (MED): now delegates to Bond::by_id (the Crud impl) instead of reimplementing the SELECT, so the trait path is exercised in production with no duplicate to diverge.
• column/bind drift (MED): the overselling comment is gone; BOND_INSERT_COLUMNS now references bond_insert_column_bind_alignment, which inserts a per-field sentinel bond and asserts each column's persisted value matches, catching a misaligned bind that value-round-trip tests would miss (the positional check #790 called the ideal option).
Runtime, migration-on-connect (the init_db.sh removal)
• Removed ~/.mostro/mostro.db and started mostrod against a fresh DB: it created the file and applied migrations on connect, coming up clean through Scheduler Started with no no such table/schema errors. Confirmed the bonds table exists in the migrated DB (sqlite3 .tables), so the bond migrations applied correctly on connect.
Live bond persistence: enabled anti_abuse_bond, created an order via mostro-cli. The maker-bond flow fired and persisted a real bond through the new Crud path, daemon logged Maker bond requested: bond_id=5706044e-… amount_sats=1000, and the DB showed that exact bond id in requested state with correct amount_sats/created_at. Confirms the create_bond INSERT via mostro_core::db::Crud works end-to-end through the live daemon, not just in unit tests. (Didn't pay the hold invoice, so the requested→locked UPDATE transition wasn't exercised live that path is covered by update_preserves_created_at.)
Scope (honest): verified build/lint/suite, the three hardening fixes via their specific tests + code paths, migration-on-connect from a fresh DB, and live bond INSERT through the daemon. Did not exercise a full multi-party dispute → slash → payout flow (needs funded LN channels + admin resolution); the bond CRUD paths it would exercise are covered by the unit tests above.
The BOND_INSERT_COLUMNS doc pointed to db.rs, but the bond_insert_column_bind_alignment test lives in bond/db.rs. Disambiguate from the top-level src/db.rs.
3 participants
Summary
sqlx-crud; usemostro_core::db::Crudfor Order/Dispute and localBondCRUD0.6.2→0.9.0(runtime-tokio,tls-rustls-ring,macros)Crudtrait)query!macros with runtimesqlx::query()— removes.sqlx/offline cache workflowinit_db.shand legacysqlx-data.json; update docs (migrations run onmostrodconnect)Separatedlifetimes,AssertSqlSafefor dynamic SQL,migration.sql.as_str()query!removal trade-offOnly five static
UPDATEhelpers insrc/db.rsusedquery!; the rest of the daemon already uses runtime SQL (query,query_as,QueryBuilder,Crud).We give up: compile-time checks on those five statements (column names, bind count, SQL syntax via
.sqlx//cargo sqlx prepare).We gain: no offline cache workflow (
init_db.sh,.sqlx/, CIDATABASE_URL), simpler sqlx 0.9 upgrade, and consistency with the rest ofdb.rs.Risk is limited: all five are fixed SQL on
orders/usersprimary keys; no generated row types were lost. Schema/query drift is still covered by migrations, integration tests, and runtime SQLite errors — same model as the other ~95% of DB code.Test plan
cargo fmt --allcargo clippy --all-targets --all-features -- -D warningscargo test(496 tests passed)cargo buildwithmostro-core0.14 from crates.ioNotes
cargo sqlx prepareorDATABASE_URLneeded for CI builds after this changeSummary by CodeRabbit
New Features
Bug Fixes
created_at.Documentation
Chores