-
Notifications
You must be signed in to change notification settings - Fork 58
fix: exclude hyphenated terminal order statuses #882
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.
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
Changes from 1 commit
4d17c8d
77d7beb
97f254e
0d7c137
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,7 +13,7 @@ use std::sync::Arc; | |
| use uuid::Uuid; | ||
|
|
||
| // Constants for status filtering used across restore session functions | ||
| const EXCLUDED_ORDER_STATUSES: &str = "'expired','success','canceled','dispute','canceledbyadmin','completedbyadmin','settledbyadmin','cooperativelycanceled'"; | ||
| const EXCLUDED_ORDER_STATUSES: &str = "'expired','success','canceled','dispute','canceled-by-admin','completed-by-admin','settled-by-admin','cooperatively-canceled'"; | ||
| const ACTIVE_DISPUTE_STATUSES: &str = "'initiated','in-progress'"; | ||
|
|
||
| /// Terminal order statuses for the Phase 2 active-trade-pubkey cache: an | ||
|
|
@@ -24,7 +24,7 @@ const ACTIVE_DISPUTE_STATUSES: &str = "'initiated','in-progress'"; | |
| /// disputed order is still active (buyer, seller and the assigned solver keep | ||
| /// messaging), so its trade keys must stay fast-pathed. See | ||
| /// `find_active_trade_pubkeys` and docs/TRANSPORT_V2_SPEC.md §6 Phase 2. | ||
| const TERMINAL_ORDER_STATUSES: &str = "'expired','success','canceled','canceledbyadmin','completedbyadmin','settledbyadmin','cooperativelycanceled'"; | ||
| const TERMINAL_ORDER_STATUSES: &str = "'expired','success','canceled','canceled-by-admin','completed-by-admin','settled-by-admin','cooperatively-canceled'"; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P2] Keep pending payout recipients in the known-key cache A canceled-by-admin order can still require a legitimate AddBondInvoice from the winning counterparty. After this change, find_active_trade_pubkeys removes that recipient's trade key on the next cache refresh if they have no other active order. On protocol v2 nodes configured with pow_first_contact > pow, accept_event then drops a response carrying the normal trade PoW before decrypting or dispatching it to add_bond_invoice_action. This can interrupt the payout even when the client has retained its session and order context. Please include the recipient keys of recoverable bond payouts in the known-key query until their obligations are resolved. This needs coverage independently of restore: a terminal order with a pending payout should retain its recipient key and allow the normal-PoW response.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 77d7beb, same clause applied to the order query in You were right to file this separately from the restore case — it is the sharper of the two. The restore gap costs a client its context; this one strands the payout even for a client that never lost its session, because the reply is dropped before decryption and nothing on either side can tell that it happened. Keying the exception on Covered independently of restore by Two notes on the diff beyond the two fixes:
Full suite 1190 passed / 0 failed / 2 ignored; |
||
|
|
||
| #[cfg(unix)] | ||
| use std::os::unix::fs::PermissionsExt; | ||
|
|
@@ -2078,6 +2078,45 @@ mod tests { | |
| None, | ||
| ) | ||
| .await; | ||
| // Terminal statuses whose serialized form is hyphenated. These are the | ||
| // ones a mis-spelled TERMINAL_ORDER_STATUSES silently fails to match, | ||
| // so they must be covered explicitly. | ||
| insert_order_with_pubkeys( | ||
| &pool, | ||
| uuid::Uuid::new_v4(), | ||
| "cooperatively-canceled", | ||
| Some("creator_coop"), | ||
| Some("buyer_coop"), | ||
| Some("seller_coop"), | ||
| ) | ||
| .await; | ||
| insert_order_with_pubkeys( | ||
| &pool, | ||
| uuid::Uuid::new_v4(), | ||
| "canceled-by-admin", | ||
| Some("creator_cba"), | ||
| None, | ||
| None, | ||
| ) | ||
| .await; | ||
| insert_order_with_pubkeys( | ||
| &pool, | ||
| uuid::Uuid::new_v4(), | ||
| "settled-by-admin", | ||
| Some("creator_sba"), | ||
| None, | ||
| None, | ||
| ) | ||
| .await; | ||
| insert_order_with_pubkeys( | ||
| &pool, | ||
| uuid::Uuid::new_v4(), | ||
| "completed-by-admin", | ||
| Some("creator_cpa"), | ||
| None, | ||
| None, | ||
| ) | ||
| .await; | ||
|
|
||
| // Active dispute with an assigned solver → solver key included. | ||
| sqlx::query( | ||
|
|
@@ -2123,6 +2162,12 @@ mod tests { | |
| "seller_succ", | ||
| "creator_canc", | ||
| "solver_settled", | ||
| "creator_coop", | ||
| "buyer_coop", | ||
| "seller_coop", | ||
| "creator_cba", | ||
| "creator_sba", | ||
| "creator_cpa", | ||
| ] { | ||
| assert!( | ||
| !keys.contains(k), | ||
|
|
@@ -2131,6 +2176,65 @@ mod tests { | |
| } | ||
| } | ||
|
|
||
| /// Restore-session must not hand back orders that are already over. | ||
| /// | ||
| /// Four of the eight excluded statuses serialize with hyphens | ||
| /// (`canceled-by-admin`, `settled-by-admin`, `completed-by-admin`, | ||
| /// `cooperatively-canceled`). A status list written without them still | ||
| /// filters the single-word ones, so the query keeps *looking* correct | ||
| /// while quietly restoring dead orders — which is why every hyphenated | ||
| /// status is asserted here individually. | ||
| #[tokio::test] | ||
| async fn find_user_orders_by_master_key_excludes_all_terminal_statuses() { | ||
| let pool = setup_orders_db().await.unwrap(); | ||
| let master_key = "a".repeat(64); | ||
|
|
||
| async fn insert_for_master(pool: &SqlitePool, status: &str, master_key: &str) { | ||
| sqlx::query( | ||
| r#" | ||
| INSERT INTO orders (id, kind, event_id, status, premium, payment_method, | ||
| amount, fiat_code, fiat_amount, created_at, expires_at, | ||
| master_buyer_pubkey, trade_index_buyer) | ||
| VALUES (?1, 'buy', 'event123', ?2, 0, 'lightning', | ||
| 100000, 'USD', 100, 1700000000, 1700086400, ?3, 1) | ||
| "#, | ||
| ) | ||
| .bind(uuid::Uuid::new_v4()) | ||
| .bind(status) | ||
| .bind(master_key) | ||
| .execute(pool) | ||
| .await | ||
| .unwrap(); | ||
| } | ||
|
|
||
| // One live order — the only row the user should get back. | ||
| insert_for_master(&pool, "waiting-payment", &master_key).await; | ||
|
|
||
| for status in [ | ||
| "expired", | ||
| "success", | ||
| "canceled", | ||
| "dispute", | ||
| "canceled-by-admin", | ||
| "completed-by-admin", | ||
| "settled-by-admin", | ||
| "cooperatively-canceled", | ||
| ] { | ||
| insert_for_master(&pool, status, &master_key).await; | ||
| } | ||
|
|
||
| let orders = super::find_user_orders_by_master_key(&pool, &master_key) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| let statuses: Vec<&str> = orders.iter().map(|o| o.status.as_str()).collect(); | ||
| assert_eq!( | ||
| statuses, | ||
| vec!["waiting-payment"], | ||
| "restore must return only the live order, got {statuses:?}" | ||
| ); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_fetch_string_column_scalar() { | ||
| let pool = setup_db().await.unwrap(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P1] Preserve restore context for outstanding bond payouts
After admin_cancel_action resolves a dispute, the order is canceled-by-admin and the dispute is seller-refunded, while apply_bond_resolution can leave a slashed bond in PendingPayout awaiting the winner's invoice. With this corrected exclusion, find_user_orders_by_master_key now drops that order; find_user_disputes_by_master_key also excludes the resolved dispute, and process_restore_session_work does not query bonds. A restoring client therefore loses the order/trade-index context needed to claim the payout. If no invoice is submitted before the claim deadline, process_one_bond forfeits the bond.
This is the admin-cancel branch of #784, but it is a behavior change introduced here: before this edit, canceled-by-admin orders were returned. Please preserve terminal orders with recoverable payouts owed to the restoring user, or land the bond-aware restore fix before enabling this exclusion. Add coverage for an admin-canceled order with an outstanding payout, alongside the terminal-order-with-no-obligation case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 77d7beb.
Both filters now carry an exception (
HAS_CLAIMABLE_BOND_PAYOUT,src/db.rs:33): a terminal order survives while a bond on it ispending-payoutorfailed.failedbelongs there becauseapply_invoicestill accepts a payout invoice against such a row inside thepayout_claim_window_dayswindow, so the claim is live even aftersend_paymentgave up. Both states are left for good on payout, slash or forfeit, so the exception retracts itself with no code to maintain.Restore therefore keeps the
canceled-by-adminorder and itstrade_index_buyer/trade_index_seller, which is what the claim needs.One deliberate limitation, worth your call: the clause is order-scoped, not recipient-scoped, so both parties see such an order rather than only the winner. Narrowing it in SQL is not really available —
bonds.pubkeyis a trade key while the restore query joins on master keys, and the Phase 6 maker-refund row (parent_bond_idset,child_order_idNULL) paysbond.pubkeyitself rather than the counterparty, so a "not the bonded side" filter would be wrong exactly where the money is owed to the bonded side. I took the over-inclusive side; say the word if you would rather have the extra complexity.Coverage as requested:
find_user_orders_by_master_key_keeps_terminal_orders_owing_a_payoutpins all four arms — terminal with no obligation (excluded),pending-payout(kept),failed(kept), andslashed(excluded, the obligation is discharged). Verified failing with the exception disabled and passing with it.