Refunder submits original UIDs after dropping failed EthFlow order-data lookups

[chenshj73]

Summary

RefundService::send_out_refunding_tx() fetches EthFlow order data for each refundable UID, logs DB lookup failures, drops the failed order data, but still submits the original full UID list to submit_batch().

The on-chain transaction uses only encoded_ethflow_orders, so any UID whose DB lookup failed is not included in invalidateOrdersIgnoringNotAllowed(...). If all lookups fail, the submitter is still called with the original UIDs and an empty order-data vector.

This creates a refund lifecycle mismatch: the service logs/submits as though a batch of UIDs is being handled, while the settlement calldata only covers the subset of orders whose DB data was successfully fetched.

Code

send_out_refunding_tx() truncates the UID list and fetches corresponding order data:

async fn send_out_refunding_tx(
    &mut self,
    uids_by_contract: HashMap<CoWSwapEthFlowAddress, Vec<OrderUid>>,
) -> Result<()> {
    if uids_by_contract.is_empty() {
        return Ok(());
    }

    // For each ethflow contract, issue a separate tx to refund
    for (contract, mut uids) in uids_by_contract.into_iter() {
        // only try to refund MAX_NUMBER_OF_UIDS_PER_REFUND_TX uids, in
        // order to fit into gas limit
        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)
            .buffer_unordered(10)
            .filter_map(|result| async {
                result
                    .inspect_err(|err| tracing::error!(?err, "failed to get data from db"))
                    .ok()
            })
            .collect()
            .await;
        self.submitter
            .submit_batch(&uids, encoded_ethflow_orders, contract)
            .await?;
    }

    Ok(())
}

submit_batch() receives both vectors, but only encoded_ethflow_orders is encoded into the contract call:

async fn submit_batch(
    &mut self,
    uids: &[OrderUid],
    encoded_ethflow_orders: Vec<EthFlowOrder::Data>,
    ethflow_contract: Address,
) -> Result<()> {
    {
        let gas_price_estimation = self.gas_estimator.estimate().await?;
        let nonce = self.get_submission_nonce().await?;
        let gas_price = calculate_submission_gas_price(
            self.gas_parameters_of_last_tx,
            gas_price_estimation,
            nonce,
            self.nonce_of_last_submission,
            self.max_gas_price,
            self.start_priority_fee_tip,
        )?;

        self.gas_parameters_of_last_tx = Some(gas_price);
        self.nonce_of_last_submission = Some(nonce);

        let ethflow_contract =
            CoWSwapEthFlow::Instance::new(ethflow_contract, self.web3.provider.clone());
        let tx_result = ethflow_contract
        .invalidateOrdersIgnoringNotAllowed(encoded_ethflow_orders)
        // Gas conversions are lossy but technically the should not have decimal points even though they're floats
        .max_priority_fee_per_gas(gas_price.max_priority_fee_per_gas)
        .max_fee_per_gas(gas_price.max_fee_per_gas)
        .from(self.signer_address)
        .nonce(nonce)
        .send()
        .await?.with_timeout(Some(TIMEOUT_5_BLOCKS)).get_receipt().await;

        match tx_result {
            Ok(receipt) => {
                tracing::debug!(
                    "Tx to refund the orderuids {:?} yielded following result {:?}",
                    uids,
                    receipt
                );
            }
            Err(err) => tracing::debug!("transaction failed with: {err}"),
        }
        Ok(())
    }
}

The tests currently document this mismatch as accepted behavior:

/// DB errors for individual orders are skipped; other orders proceed.
///
/// # Current Behavior (documented, not necessarily ideal)
///
/// 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
///
/// This means `submit` receives:
/// - `uids`: ALL original UIDs (including those with failed 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;
    let uid2 = create_test_order_placement(2, KNOWN_ETHFLOW).uid;

    let mut mock_db = MockDbRead::new();

    // First order (uid_suffix=1) fails DB lookup to test error handling
    mock_db
        .expect_get_ethflow_order_data()
        .withf(|uid| uid.0[31] == 1)
        .returning(|_| Err(anyhow!("DB error")));

    // Second order (uid_suffix=2) succeeds to verify partial success
    // behavior
    mock_db
        .expect_get_ethflow_order_data()
        .withf(|uid| uid.0[31] == 2)
        .returning(|_| Ok(EthFlowOrder::Data::default()));

    let mock_chain = MockChainRead::new();

    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)
    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
        })
        .returning(|_, _, _| Ok(()));

The all-failed case is also explicitly covered:

/// If every DB lookup fails, we still call the submitter with the original
/// UIDs but without any order data.
///
/// What actually happens:
/// - 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.
///
/// 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.
#[tokio::test]
async fn test_send_out_refunding_tx_all_db_calls_fail_still_submits() {

Why this matters

The UID vector is used for observability and control flow, while the order-data vector is what actually drives the on-chain invalidation/refund call. Once those vectors diverge, a failed DB lookup can make an eligible refund silently disappear from the on-chain transaction while the batch is still treated as successfully submitted.

In the all-failed case, submit_batch() can send invalidateOrdersIgnoringNotAllowed([]) and still return Ok(()) after logging the transaction result. That makes it harder for the service to retry the missing refund data as a failed lifecycle transition.

Suggested fix

Keep UIDs and fetched order data paired through the fetch step. For example:

• collect (uid, order_data) only for successful lookups and pass only those UIDs to submit_batch();
• skip the on-chain submission if the resulting order-data vector is empty;
• return an error, or otherwise mark the corresponding UIDs for retry, when all order-data lookups fail.

This would make the refunder's local UID state match the actual calldata submitted on-chain.