fix(twap-monitor): skip submit_order when submitted:{uid} already in store (COW-1085)

modules/twap-monitor/src/strategy.rs

@@ -11,8 +11,8 @@
 use alloy_primitives::{Address, B256, Bytes, keccak256};
 use alloy_sol_types::{SolCall, SolEvent, SolValue};
 use cowprotocol::{
-    COMPOSABLE_COW, ComposableCoW::ConditionalOrderCreated, ConditionalOrderParams, GPv2OrderData,
-    OrderCreation, Signature,
+    COMPOSABLE_COW, Chain, ComposableCoW::ConditionalOrderCreated, ConditionalOrderParams,
+    GPv2OrderData, OrderCreation, Signature,
 };
 use shepherd_sdk::chain::{eth_call_params, parse_eth_call_result};
 use shepherd_sdk::cow::{PollOutcome, RetryAction, classify_api_error, gpv2_to_order_data};
@@ -329,6 +329,29 @@ fn submit_ready<H: Host>(
     watch_key: &str,
     now_epoch_s: u64,
 ) -> Result<(), HostError> {
+    // COW-1085: short-circuit if the orderbook UID for this exact
+    // (order, owner, chain) tuple is already in our local-store as
+    // `submitted:`. The poll-tick can re-fire `Ready` for the same
+    // TWAP child in successive blocks — `getTradeableOrderWithSignature`
+    // does not know shepherd already POSTed it — and re-submitting
+    // wastes an appData GET + submit_order call and emits a
+    // misleading `DuplicatedOrder` Warn. The UID computation is
+    // deterministic from on-chain inputs (and matches what the
+    // orderbook derives server-side from the signed payload), so we
+    // can check before doing any network work. We also reuse the
+    // computed value below as the `submitted:{uid}` marker key, so
+    // the read and write paths agree.
+    let client_uid_hex = compute_uid_hex(chain_id, order, owner);
+    if let Some(uid_hex) = client_uid_hex.as_deref()
+        && host.get(&format!("submitted:{uid_hex}"))?.is_some()
+    {
+        host.log(
+            LogLevel::Info,
+            &format!("twap {uid_hex} already submitted; skipping poll re-submit"),
+        );
+        return Ok(());
+    }
+
     // COW-1074: cow-swap UI (and other clients) sign TWAPs with a
     // non-empty `appData` hash that points at a JSON document held
     // by the orderbook's app_data registry. Hard-coding
@@ -385,10 +408,30 @@ fn submit_ready<H: Host>(
         }
     };
     match host.submit_order(chain_id, &body) {
-        Ok(uid) => {
-            let key = format!("submitted:{uid}");
+        Ok(server_uid) => {
+            // Prefer the client-computed UID for the marker key so the
+            // idempotency check at the top of `submit_ready` reads what
+            // we wrote (COW-1085). In production the server-returned
+            // UID is the same value (both sides derive it from the
+            // signed `OrderData` via the canonical
+            // `digest || owner || valid_to` layout); a divergence
+            // would be a protocol-level bug worth surfacing rather
+            // than silently splitting the keyspace.
+            let marker_uid = client_uid_hex.as_deref().unwrap_or(server_uid.as_str());
+            let key = format!("submitted:{marker_uid}");
             // Empty marker - presence of the key is the receipt.
             host.set(&key, b"")?;
+            if let Some(client_uid) = client_uid_hex.as_deref()
+                && client_uid != server_uid
+            {
+                host.log(
+                    LogLevel::Warn,
+                    &format!(
+                        "twap UID divergence: client={client_uid} server={server_uid} \
+                         (marker stored under client UID for idempotency consistency)"
+                    ),
+                );
+            }
             host.log(LogLevel::Info, &format!("submitted {key}"));
         }
         Err(err) => {
@@ -398,6 +441,23 @@ fn submit_ready<H: Host>(
     Ok(())
 }
 
+/// Compute the orderbook UID hex (`0x` + 112 hex chars) for the given
+/// on-chain (order, owner, chain) tuple, mirroring what `submit_order`
+/// will deduce server-side. Used by [`submit_ready`] to short-circuit
+/// poll-tick re-submissions of an already-submitted TWAP child
+/// (COW-1085).
+///
+/// Returns `None` if the chain id is unsupported by `cowprotocol::Chain`
+/// or the order carries an unknown enum marker — both cases also stop
+/// the regular submit path downstream, so the caller can fall through
+/// to the normal flow and let it surface the appropriate diagnostic.
+fn compute_uid_hex(chain_id: u64, order: &GPv2OrderData, owner: Address) -> Option<String> {
+    let chain = Chain::try_from(chain_id).ok()?;
+    let domain = chain.settlement_domain();
+    let order_data = gpv2_to_order_data(order)?;
+    Some(format!("{}", order_data.uid(&domain, owner)))
+}
+
 // ---- BLEU-829: OrderPostError -> retry action ----
 
 fn apply_submit_retry<H: Host>(
@@ -888,11 +948,80 @@ mod tests {
 
         on_block(&host, sample_block(1_000)).unwrap();
 
+        let expected_uid = compute_uid_hex(SEPOLIA, &ready_order, owner)
+            .expect("Sepolia is supported + canonical markers");
         assert_eq!(host.chain.call_count(), 1);
         assert_eq!(host.cow_api.call_count(), 1);
         assert!(
-            host.store.snapshot().contains_key("submitted:0xfeedface"),
-            "expected submitted:{{uid}} marker"
+            host.store
+                .snapshot()
+                .contains_key(&format!("submitted:{expected_uid}")),
+            "expected submitted:{{client_uid}} marker (COW-1085: marker key now uses the client-computed UID, not the server-returned one, so the idempotency check at the top of submit_ready reads what we wrote)"
+        );
+        // The MockHost orderbook stub returns `0xfeedface` instead of
+        // the canonical UID; this asserts the strategy logs a Warn
+        // about the divergence (real orderbooks would not diverge).
+        assert!(
+            host.logging.contains("twap UID divergence"),
+            "expected divergence Warn when mock orderbook returns a non-canonical UID"
+        );
+    }
+
+    /// COW-1085 regression guard: when `getTradeableOrderWithSignature`
+    /// returns the same Ready tuple in consecutive poll-ticks (the
+    /// on-chain conditional order does not know shepherd already
+    /// POSTed it), the second tick must NOT call `submit_order`
+    /// again. Without the guard the orderbook responds with
+    /// `DuplicatedOrder` and a Warn fires for what is in fact
+    /// correct, finished work. The guard is the `submitted:{uid}`
+    /// short-circuit at the top of `submit_ready`.
+    #[test]
+    fn poll_ready_skips_submit_when_submitted_uid_already_in_store() {
+        let host = MockHost::new();
+        let owner = address!("0011223344556677889900AABBCCDDEEFF001122");
+        let params = sample_params();
+        seed_watch(&host, owner, &params);
+
+        let ready_order = submittable_order();
+        let signature: Bytes = hex!("c0ffeec0ffeec0ffee").to_vec().into();
+        let wire = (ready_order.clone(), signature.clone()).abi_encode_params();
+        host.chain.respond_to(
+            "eth_call",
+            programmed_eth_call_params(owner, &params),
+            Ok(quoted_hex(&wire)),
+        );
+
+        // Seed the marker that a previous successful poll-tick would
+        // have written. The poll path must read this and skip; the
+        // orderbook submit must not be attempted.
+        let already_submitted_uid = compute_uid_hex(SEPOLIA, &ready_order, owner)
+            .expect("Sepolia is supported + canonical markers");
+        host.store
+            .set(&format!("submitted:{already_submitted_uid}"), b"")
+            .expect("seed submitted marker");
+
+        on_block(&host, sample_block(1_000)).unwrap();
+
+        assert_eq!(
+            host.chain.call_count(),
+            1,
+            "poll still consults the chain to see Ready",
+        );
+        assert_eq!(
+            host.cow_api.call_count(),
+            0,
+            "submit_order must NOT be called when submitted:{{uid}} already exists",
+        );
+        assert_eq!(
+            host.cow_api.request_calls().len(),
+            0,
+            "appData resolve must NOT be called either — the guard short-circuits early",
+        );
+        assert!(
+            host.logging.contains(&format!(
+                "twap {already_submitted_uid} already submitted; skipping poll re-submit"
+            )),
+            "expected the idempotency-skip Info log line",
         );
     }
 
@@ -953,9 +1082,13 @@ mod tests {
             1,
             "exactly one app_data resolve",
         );
+        let expected_uid = compute_uid_hex(SEPOLIA, &ready_order, owner)
+            .expect("Sepolia is supported + canonical markers");
         assert!(
-            host.store.snapshot().contains_key("submitted:0xfeedface"),
-            "submitted:{{uid}} marker must be written after a successful resolve+submit"
+            host.store
+                .snapshot()
+                .contains_key(&format!("submitted:{expected_uid}")),
+            "submitted:{{client_uid}} marker must be written after a successful resolve+submit (COW-1085)"
         );
     }