Introduce Quality::Unknown variant (#3206)

# Description
Sparked by this
[discussion](#3205 (comment))
I think that using `Option<Quality>` is a bit implicit.

# Changes
I adjusted the bad token detection code to use `Quality::Unknown` where
possible to make it more explicit what the strategy thinks about a
token.

This makes some of the code more verbose so I'm not super sold on this
change yet. Let me know if you think this improves things.

crates/driver/src/domain/competition/bad_tokens/cache.rs

@@ -25,7 +25,7 @@ struct CacheEntry {
     /// when the decision on the token quality was made
     last_updated: Instant,
     /// whether the token is supported or not
-    quality: Quality,
+    is_supported: bool,
 }
 
 impl Cache {
@@ -39,23 +39,21 @@ impl Cache {
     }
 
     /// Updates whether or not a token should be considered supported.
-    pub fn update_quality(&self, token: eth::TokenAddress, quality: Quality, now: Instant) {
+    pub fn update_quality(&self, token: eth::TokenAddress, is_supported: bool, now: Instant) {
         self.0
             .cache
             .entry(token)
-            .and_modify(|value| {
-                if quality == Quality::Unsupported
-                    || now.duration_since(value.last_updated) > self.0.max_age
-                {
+            .and_modify(|token| {
+                if !is_supported || now.duration_since(token.last_updated) > self.0.max_age {
                     // Only update the value if the cached value is outdated by now or
                     // if the new value is "Unsupported". This means on conflicting updates
                     // we err on the conservative side and assume a token is unsupported.
-                    value.quality = quality;
+                    token.is_supported = is_supported;
                 }
-                value.last_updated = now;
+                token.last_updated = now;
             })
             .or_insert_with(|| CacheEntry {
-                quality,
+                is_supported,
                 last_updated: now,
             });
     }
@@ -69,9 +67,15 @@ impl Cache {
 
     /// Returns the quality of the token if the cached value has not expired
     /// yet.
-    pub fn get_quality(&self, token: &eth::TokenAddress, now: Instant) -> Option<Quality> {
-        let token = self.0.cache.get(token)?;
+    pub fn get_quality(&self, token: &eth::TokenAddress, now: Instant) -> Quality {
+        let Some(token) = self.0.cache.get(token) else {
+            return Quality::Unknown;
+        };
         let still_valid = now.duration_since(token.last_updated) > self.0.max_age;
-        still_valid.then_some(token.quality)
+        match (still_valid, token.is_supported) {
+            (false, _) => Quality::Unknown,
+            (true, true) => Quality::Supported,
+            (true, false) => Quality::Unsupported,
+        }
     }
 }

crates/driver/src/domain/competition/bad_tokens/metrics.rs

@@ -45,27 +45,36 @@ impl Detector {
         }
     }
 
-    pub fn get_quality(&self, token: &eth::TokenAddress, now: Instant) -> Option<Quality> {
-        let stats = self.counter.get(token)?;
+    pub fn get_quality(&self, token: &eth::TokenAddress, now: Instant) -> Quality {
+        let Some(stats) = self.counter.get(token) else {
+            return Quality::Unknown;
+        };
+
         if stats
             .flagged_unsupported_at
             .is_some_and(|t| now.duration_since(t) > self.token_freeze_time)
         {
             // Sometimes tokens only cause issues temporarily. If the token's freeze
-            // period expired we give it another chance to see if it still behaves badly.
-            return None;
+            // period expired we pretend we don't have enough information to give it
+            // another chance. If it still behaves badly it will get frozen immediately.
+            return Quality::Unknown;
         }
 
-        let is_unsupported = self.stats_indicate_unsupported(&stats);
-        (!self.log_only && is_unsupported).then_some(Quality::Unsupported)
+        match self.log_only {
+            true => Quality::Supported,
+            false => self.quality_based_on_stats(&stats),
+        }
     }
 
-    fn stats_indicate_unsupported(&self, stats: &TokenStatistics) -> bool {
-        let token_failure_ratio = match stats.attempts {
-            0 => return false,
-            attempts => f64::from(stats.fails) / f64::from(attempts),
-        };
-        stats.attempts >= self.required_measurements && token_failure_ratio >= self.failure_ratio
+    fn quality_based_on_stats(&self, stats: &TokenStatistics) -> Quality {
+        if stats.attempts < self.required_measurements {
+            return Quality::Unknown;
+        }
+        let token_failure_ratio = f64::from(stats.fails) / f64::from(stats.attempts);
+        match token_failure_ratio >= self.failure_ratio {
+            true => Quality::Unsupported,
+            false => Quality::Supported,
+        }
     }
 
     /// Updates the tokens that participated in settlements by
@@ -96,7 +105,7 @@ impl Detector {
                     });
 
                 // token neeeds to be frozen as unsupported for a while
-                if self.stats_indicate_unsupported(&stats)
+                if self.quality_based_on_stats(&stats) == Quality::Unsupported
                     && stats
                         .flagged_unsupported_at
                         .is_none_or(|t| now.duration_since(t) > self.token_freeze_time)
@@ -128,28 +137,23 @@ mod tests {
 
         let token_a = eth::TokenAddress(eth::ContractAddress(H160([1; 20])));
         let token_b = eth::TokenAddress(eth::ContractAddress(H160([2; 20])));
+        let token_quality = || detector.get_quality(&token_a, Instant::now());
 
-        // token is reported as supported while we don't have enough measurements
-        assert_eq!(detector.get_quality(&token_a, Instant::now()), None);
+        // token is reported as unknown while we don't have enough measurements
+        assert_eq!(token_quality(), Quality::Unknown);
         detector.update_tokens(&[(token_a, token_b)], true);
-        assert_eq!(detector.get_quality(&token_a, Instant::now()), None);
+        assert_eq!(token_quality(), Quality::Unknown);
         detector.update_tokens(&[(token_a, token_b)], true);
 
         // after we got enough measurements the token gets marked as bad
-        assert_eq!(
-            detector.get_quality(&token_a, Instant::now()),
-            Some(Quality::Unsupported)
-        );
+        assert_eq!(token_quality(), Quality::Unsupported);
 
-        // after the freeze period is over the token gets reported as good again
+        // after the freeze period is over the token gets reported as unknown again
         tokio::time::sleep(FREEZE_DURATION).await;
-        assert_eq!(detector.get_quality(&token_a, Instant::now()), None);
+        assert_eq!(token_quality(), Quality::Unknown);
 
         // after an unfreeze another bad measurement is enough to freeze it again
         detector.update_tokens(&[(token_a, token_b)], true);
-        assert_eq!(
-            detector.get_quality(&token_a, Instant::now()),
-            Some(Quality::Unsupported)
-        );
+        assert_eq!(token_quality(), Quality::Unsupported);
     }
 }

crates/driver/src/domain/competition/bad_tokens/mod.rs

@@ -23,6 +23,9 @@ pub enum Quality {
     ///   contract - see <https://github.com/cowprotocol/services/pull/781>
     /// * probably tons of other reasons
     Unsupported,
+    /// The detection strategy does not have enough data to make an informed
+    /// decision.
+    Unknown,
 }
 
 #[derive(Default)]
@@ -67,26 +70,23 @@ impl Detector {
             let buy = self.get_token_quality(order.buy.token, now);
             match (sell, buy) {
                 // both tokens supported => keep order
-                (Some(Quality::Supported), Some(Quality::Supported)) => Either::Left(order),
+                (Quality::Supported, Quality::Supported) => Either::Left(order),
                 // at least 1 token unsupported => drop order
-                (Some(Quality::Unsupported), _) | (_, Some(Quality::Unsupported)) => {
-                    Either::Right(order.uid)
-                }
+                (Quality::Unsupported, _) | (_, Quality::Unsupported) => Either::Right(order.uid),
                 // sell token quality is unknown => keep order if token is supported
-                (None, _) => {
+                (Quality::Unknown, _) => {
                     let Some(detector) = &self.simulation_detector else {
                         // we can't determine quality => assume order is good
                         return Either::Left(order);
                     };
-                    let quality = detector.determine_sell_token_quality(&order, now).await;
-                    match quality {
-                        Some(Quality::Supported) => Either::Left(order),
+                    match detector.determine_sell_token_quality(&order, now).await {
+                        Quality::Supported => Either::Left(order),
                         _ => Either::Right(order.uid),
                     }
                 }
                 // buy token quality is unknown => keep order (because we can't
                 // determine quality and assume it's good)
-                (_, None) => Either::Left(order),
+                (_, Quality::Unknown) => Either::Left(order),
             }
         });
         let (supported_orders, removed_uids): (Vec<_>, Vec<_>) = join_all(token_quality_checks)
@@ -120,22 +120,27 @@ impl Detector {
         }
     }
 
-    fn get_token_quality(&self, token: eth::TokenAddress, now: Instant) -> Option<Quality> {
-        if let Some(quality) = self.hardcoded.get(&token) {
-            return Some(*quality);
+    fn get_token_quality(&self, token: eth::TokenAddress, now: Instant) -> Quality {
+        match self.hardcoded.get(&token) {
+            None | Some(Quality::Unknown) => (),
+            Some(quality) => return *quality,
         }
 
-        if let Some(detector) = &self.simulation_detector {
-            if let Some(quality) = detector.get_quality(&token, now) {
-                return Some(quality);
-            }
+        if let Some(Quality::Unsupported) = self
+            .simulation_detector
+            .as_ref()
+            .map(|d| d.get_quality(&token, now))
+        {
+            return Quality::Unsupported;
         }
 
-        if let Some(metrics) = &self.metrics {
-            return metrics.get_quality(&token, now);
+        if let Some(Quality::Unsupported) =
+            self.metrics.as_ref().map(|m| m.get_quality(&token, now))
+        {
+            return Quality::Unsupported;
         }
 
-        None
+        Quality::Unknown
     }
 }
 

crates/driver/src/domain/competition/bad_tokens/simulation.rs

@@ -32,7 +32,7 @@ pub struct Detector(Arc<Inner>);
 struct Inner {
     cache: Cache,
     detector: TraceCallDetectorRaw,
-    sharing: BoxRequestSharing<order::Uid, Option<Quality>>,
+    sharing: BoxRequestSharing<order::Uid, Quality>,
 }
 
 impl Detector {
@@ -49,14 +49,11 @@ impl Detector {
     /// Simulates how the sell token behaves during transfers. Assumes that
     /// the order owner has the required sell token balance and approvals
     /// set.
-    pub async fn determine_sell_token_quality(
-        &self,
-        order: &Order,
-        now: Instant,
-    ) -> Option<Quality> {
+    pub async fn determine_sell_token_quality(&self, order: &Order, now: Instant) -> Quality {
         let cache = &self.0.cache;
-        if let Some(quality) = cache.get_quality(&order.sell.token, now) {
-            return Some(quality);
+        let quality = cache.get_quality(&order.sell.token, now);
+        if quality != Quality::Unknown {
+            return quality;
         }
 
         // The simulation detector gets used by multiple solvers at the same time
@@ -93,20 +90,20 @@ impl Detector {
                     match result {
                         Err(err) => {
                             tracing::debug!(?err, token=?sell_token.0, "failed to determine token quality");
-                            None
+                            Quality::Unknown
                         }
                         Ok(TokenQuality::Good) => {
                             inner
                                 .cache
-                                .update_quality(sell_token, Quality::Supported, now);
-                            Some(Quality::Supported)
+                                .update_quality(sell_token, true, now);
+                            Quality::Supported
                         }
                         Ok(TokenQuality::Bad { reason }) => {
                             tracing::debug!(reason, token=?sell_token.0, "cache token as unsupported");
                             inner
                                 .cache
-                                .update_quality(sell_token, Quality::Unsupported, now);
-                            Some(Quality::Unsupported)
+                                .update_quality(sell_token, false, now);
+                            Quality::Unsupported
                         }
                     }
                 }