From 484f3d54ca130dc648945148b930f76fd845ce27 Mon Sep 17 00:00:00 2001 From: metalurgical <97008724+metalurgical@users.noreply.github.com> Date: Thu, 14 May 2026 17:51:29 +0200 Subject: [PATCH] fix(refunder): align submitted UIDs with loaded order data Keep refund submission UIDs paired with successfully loaded ETHFlow order data. Additionally, skip empty submissions and improve DB error logging. --- crates/refunder/src/refund_service.rs | 96 ++++++++++++--------------- 1 file changed, 43 insertions(+), 53 deletions(-) diff --git a/crates/refunder/src/refund_service.rs b/crates/refunder/src/refund_service.rs index 1bdeaeba23..01b87dde00 100644 --- a/crates/refunder/src/refund_service.rs +++ b/crates/refunder/src/refund_service.rs @@ -207,20 +207,34 @@ where uids.truncate(MAX_NUMBER_OF_UIDS_PER_REFUND_TX); tracing::debug!("Trying to refund the following uids: {:?}", uids); - - let futures = uids.iter().map(|uid| { - let (uid, database) = (*uid, &self.database); - async move { database.get_ethflow_order_data(&uid).await } - }); - let encoded_ethflow_orders: Vec<_> = stream::iter(futures) + let (uids, encoded_ethflow_orders): (Vec<_>, Vec<_>) = stream::iter(uids.iter()) + .map(|uid| { + let (uid, database) = (*uid, &self.database); + async move { + database + .get_ethflow_order_data(&uid) + .await + .map(|order| (uid, order)) + .map_err(|err| (uid, err)) + } + }) .buffer_unordered(10) .filter_map(|result| async { result - .inspect_err(|err| tracing::error!(?err, "failed to get data from db")) + .inspect_err(|(uid, err)| { + tracing::error!(?uid, ?err, "failed to get data from db") + }) .ok() }) - .collect() - .await; + .collect::>() + .await + .into_iter() + .unzip(); + + if uids.is_empty() { + continue; + } + self.submitter .submit_batch(&uids, encoded_ethflow_orders, contract) .await?; @@ -305,7 +319,7 @@ mod tests { /// Asserts the expected number of orders per contract in a grouped result. /// /// # Panics - /// Panics if the number of orders for the specified contract does not match + /// If the number of orders for the specified contract does not match /// the expected count, or if the set of UID suffixes differs. #[track_caller] fn assert_orders_by_contract( @@ -582,19 +596,15 @@ mod tests { /// DB errors for individual orders are skipped; other orders proceed. /// - /// # Current Behavior (documented, not necessarily ideal) + /// # Behavior /// /// When a DB lookup fails for an order: /// - The error is logged and the order data is excluded from the submission - /// - However, the UID is still included in the submission + /// - The corresponding UID is also excluded from the submission /// - /// This means `submit` receives: - /// - `uids`: ALL original UIDs (including those with failed lookups) + /// This keeps `submit` inputs aligned: + /// - `uids`: Only UIDs with successful lookups /// - `orders`: Only the order data for successful lookups - /// - /// This creates a mismatch between UIDs and order data. See the TODO in - /// `test_send_out_refunding_tx_all_db_calls_fail_still_submits` for - /// discussion of potential fixes. #[tokio::test] async fn test_send_out_refunding_tx_db_error_skips_order() { let uid1 = create_test_order_placement(1, KNOWN_ETHFLOW).uid; @@ -618,18 +628,17 @@ mod tests { let mut mock_submitter = MockChainWrite::new(); - // Current behavior: ALL UIDs are passed, but only successful order data. - // - uids contains both uid1 (suffix=1) and uid2 (suffix=2) - // - orders contains only 1 entry (from uid2's successful lookup) + // Only successfully loaded UIDs are passed with their matching order data. mock_submitter .expect_submit_batch() .times(1) - .withf(|uids, orders, _| { - let has_both_uids = uids.len() == 2 - && uids.iter().any(|uid| uid.0[31] == 1) - && uids.iter().any(|uid| uid.0[31] == 2); - let has_one_order = orders.len() == 1; - has_both_uids && has_one_order + .withf(|uids, orders, contract| { + let submitted_uid_suffixes = + uids.iter().map(|uid| uid.0[31]).collect::>(); + uids.len() == orders.len() + && orders.len() == 1 + && submitted_uid_suffixes == HashSet::from([2]) + && *contract == KNOWN_ETHFLOW }) .returning(|_, _, _| Ok(())); @@ -642,30 +651,20 @@ mod tests { assert!(result.is_ok()); } - /// If every DB lookup fails, we still call the submitter with the original - /// UIDs but without any order data. + /// If every DB lookup fails, no refund transaction is submitted. /// /// What actually happens: - /// - Each failed order‑data fetch is logged and ignored (it doesn't stop + /// - Each failed order-data fetch is logged and ignored (it doesn't stop /// the whole batch). - /// - The submitter gets the same list of UIDs we started with, but the - /// `orders` slice may be empty (or contain fewer entries) because some or - /// all lookups failed. - /// - /// TODO: Is this the behavior we really want? Submitting a refund that - /// contains UIDs but no order details feels off. Possible fixes: - /// 1. Skip the submission entirely when `encoded_ethflow_orders` is empty. - /// 2. Return an error if *all* order‑data lookups fail. - /// 3. Filter the UID list so it only includes IDs with successful lookups. + /// - Since no order data is available, the submitter is not called. /// /// NOTE: This test complements /// `test_send_out_refunding_tx_db_error_skips_order`. That test covers /// partial DB failure (some lookups succeed); this one covers /// total DB failure (all lookups fail). Together they verify that DB errors - /// are non-fatal and UIDs are always preserved regardless of lookup - /// success. + /// are non-fatal and only UIDs with successful lookups are submitted. #[tokio::test] - async fn test_send_out_refunding_tx_all_db_calls_fail_still_submits() { + async fn test_send_out_refunding_tx_all_db_calls_fail_skips_submission() { let uid1 = create_test_order_placement(1, KNOWN_ETHFLOW).uid; let uid2 = create_test_order_placement(2, KNOWN_ETHFLOW).uid; @@ -680,17 +679,8 @@ mod tests { let mut mock_submitter = MockChainWrite::new(); - // Verify submission still happens with original UIDs but empty orders list - // This documents current (possibly unintended) behavior where UIDs and orders - // mismatch - mock_submitter - .expect_submit_batch() - .times(1) - .withf(|uids, orders, contract| { - // UIDs are preserved, but orders is empty because all DB lookups failed - uids.len() == 2 && orders.is_empty() && *contract == KNOWN_ETHFLOW - }) - .returning(|_, _, _| Ok(())); + // Verify submission does not happen when no order data could be loaded. + mock_submitter.expect_submit_batch().times(0); let mut service = RefundService::new(mock_db, mock_chain, mock_submitter, 3600, 100);