Reduce Try/TryFrom implementations for general types (#3130)

# Description During some debugging sessions, it was noticed that having implicit conversions overcomplicate code navigations. The proposal is to use explicit `from/into_domain` functions for general types. # Changes The PR touches only Auction and FeePolicy structs at the moment. But once bumped into the same issue with any other struct, a follow-up should be opened.
cowprotocol · Nov 21, 2024 · 722b1ce · 722b1ce
1 parent 27e3c3b
commit 722b1ce
Show file tree

Hide file tree

Showing 6 changed files with 81 additions and 76 deletions.
diff --git a/crates/autopilot/src/domain/fee/mod.rs b/crates/autopilot/src/domain/fee/mod.rs
@@ -288,8 +288,8 @@ pub struct Quote {
     pub solver: H160,
 }
 
-impl From<domain::Quote> for Quote {
-    fn from(value: domain::Quote) -> Self {
+impl Quote {
+    fn from_domain(value: &domain::Quote) -> Self {
         Self {
             sell_amount: value.sell_amount.into(),
             buy_amount: value.buy_amount.into(),

diff --git a/crates/autopilot/src/domain/fee/policy.rs b/crates/autopilot/src/domain/fee/policy.rs
@@ -1,7 +1,10 @@
 use crate::{
     arguments,
     boundary,
-    domain::{self, fee::FeeFactor},
+    domain::{
+        self,
+        fee::{FeeFactor, Quote},
+    },
 };
 
 pub enum Policy {
@@ -74,7 +77,7 @@ impl PriceImprovement {
             boundary::OrderClass::Limit => Some(domain::fee::Policy::PriceImprovement {
                 factor: self.factor,
                 max_volume_factor: self.max_volume_factor,
-                quote: quote.clone().into(),
+                quote: Quote::from_domain(quote),
             }),
         }
     }

diff --git a/crates/autopilot/src/infra/persistence/dto/auction.rs b/crates/autopilot/src/infra/persistence/dto/auction.rs
@@ -48,29 +48,36 @@ pub struct RawAuctionData {
 
 pub type AuctionId = i64;
 
-impl TryFrom<Auction> for domain::Auction {
-    type Error = anyhow::Error;
+#[serde_as]
+#[derive(Clone, Debug, Deserialize)]
+#[serde(rename_all = "camelCase")]
+pub struct Auction {
+    pub id: AuctionId,
+    #[serde(flatten)]
+    pub auction: RawAuctionData,
+}
 
-    fn try_from(dto: Auction) -> anyhow::Result<Self> {
+impl Auction {
+    pub fn try_into_domain(self) -> anyhow::Result<domain::Auction> {
         Ok(domain::Auction {
-            id: dto.id,
-            block: dto.auction.block,
-            latest_settlement_block: dto.auction.latest_settlement_block,
-            orders: dto
+            id: self.id,
+            block: self.auction.block,
+            latest_settlement_block: self.auction.latest_settlement_block,
+            orders: self
                 .auction
                 .orders
                 .into_iter()
                 .map(super::order::to_domain)
                 .collect(),
-            prices: dto
+            prices: self
                 .auction
                 .prices
                 .into_iter()
                 .map(|(key, value)| {
                     Price::new(value.into()).map(|price| (eth::TokenAddress(key), price))
                 })
                 .collect::<Result<_, _>>()?,
-            surplus_capturing_jit_order_owners: dto
+            surplus_capturing_jit_order_owners: self
                 .auction
                 .surplus_capturing_jit_order_owners
                 .into_iter()
@@ -79,12 +86,3 @@ impl TryFrom<Auction> for domain::Auction {
         })
     }
 }
-
-#[serde_as]
-#[derive(Clone, Debug, Deserialize)]
-#[serde(rename_all = "camelCase")]
-pub struct Auction {
-    pub id: AuctionId,
-    #[serde(flatten)]
-    pub auction: RawAuctionData,
-}
diff --git a/crates/autopilot/src/infra/persistence/dto/order.rs b/crates/autopilot/src/infra/persistence/dto/order.rs
@@ -49,7 +49,11 @@ pub fn from_domain(order: domain::Order) -> Order {
         buy_token: order.buy.token.into(),
         sell_amount: order.sell.amount.into(),
         buy_amount: order.buy.amount.into(),
-        protocol_fees: order.protocol_fees.into_iter().map(Into::into).collect(),
+        protocol_fees: order
+            .protocol_fees
+            .into_iter()
+            .map(FeePolicy::from_domain)
+            .collect(),
         created: order.created,
         valid_to: order.valid_to,
         kind: order.side.into(),
@@ -68,7 +72,7 @@ pub fn from_domain(order: domain::Order) -> Order {
         class: boundary::OrderClass::Limit,
         app_data: order.app_data.into(),
         signature: order.signature.into(),
-        quote: order.quote.map(Into::into),
+        quote: order.quote.map(Quote::from_domain),
     }
 }
 
@@ -83,7 +87,11 @@ pub fn to_domain(order: Order) -> domain::Order {
             token: order.buy_token.into(),
             amount: order.buy_amount.into(),
         },
-        protocol_fees: order.protocol_fees.into_iter().map(Into::into).collect(),
+        protocol_fees: order
+            .protocol_fees
+            .into_iter()
+            .map(FeePolicy::into_domain)
+            .collect(),
         created: order.created,
         valid_to: order.valid_to,
         side: order.kind.into(),
@@ -262,44 +270,8 @@ pub enum FeePolicy {
     Volume { factor: f64 },
 }
 
-#[serde_as]
-#[derive(Clone, Debug, Serialize, Deserialize)]
-#[serde(rename_all = "camelCase")]
-pub struct Quote {
-    #[serde_as(as = "HexOrDecimalU256")]
-    pub sell_amount: U256,
-    #[serde_as(as = "HexOrDecimalU256")]
-    pub buy_amount: U256,
-    #[serde_as(as = "HexOrDecimalU256")]
-    pub fee: U256,
-    pub solver: H160,
-}
-
-impl Quote {
-    pub fn to_domain(&self, order_uid: OrderUid) -> domain::Quote {
-        domain::Quote {
-            order_uid,
-            sell_amount: self.sell_amount.into(),
-            buy_amount: self.buy_amount.into(),
-            fee: self.fee.into(),
-            solver: self.solver.into(),
-        }
-    }
-}
-
-impl From<domain::Quote> for Quote {
-    fn from(quote: domain::Quote) -> Self {
-        Quote {
-            sell_amount: quote.sell_amount.0,
-            buy_amount: quote.buy_amount.0,
-            fee: quote.fee.0,
-            solver: quote.solver.0,
-        }
-    }
-}
-
-impl From<domain::fee::Policy> for FeePolicy {
-    fn from(policy: domain::fee::Policy) -> Self {
+impl FeePolicy {
+    pub fn from_domain(policy: domain::fee::Policy) -> Self {
         match policy {
             domain::fee::Policy::Surplus {
                 factor,
@@ -327,23 +299,21 @@ impl From<domain::fee::Policy> for FeePolicy {
             },
         }
     }
-}
 
-impl From<FeePolicy> for domain::fee::Policy {
-    fn from(policy: FeePolicy) -> Self {
-        match policy {
-            FeePolicy::Surplus {
+    pub fn into_domain(self) -> domain::fee::Policy {
+        match self {
+            Self::Surplus {
                 factor,
                 max_volume_factor,
-            } => Self::Surplus {
+            } => domain::fee::Policy::Surplus {
                 factor: FeeFactor::try_from(factor).unwrap(),
                 max_volume_factor: FeeFactor::try_from(max_volume_factor).unwrap(),
             },
-            FeePolicy::PriceImprovement {
+            Self::PriceImprovement {
                 factor,
                 max_volume_factor,
                 quote,
-            } => Self::PriceImprovement {
+            } => domain::fee::Policy::PriceImprovement {
                 factor: FeeFactor::try_from(factor).unwrap(),
                 max_volume_factor: FeeFactor::try_from(max_volume_factor).unwrap(),
                 quote: domain::fee::Quote {
@@ -353,13 +323,47 @@ impl From<FeePolicy> for domain::fee::Policy {
                     solver: quote.solver,
                 },
             },
-            FeePolicy::Volume { factor } => Self::Volume {
+            Self::Volume { factor } => domain::fee::Policy::Volume {
                 factor: FeeFactor::try_from(factor).unwrap(),
             },
         }
     }
 }
 
+#[serde_as]
+#[derive(Clone, Debug, Serialize, Deserialize)]
+#[serde(rename_all = "camelCase")]
+pub struct Quote {
+    #[serde_as(as = "HexOrDecimalU256")]
+    pub sell_amount: U256,
+    #[serde_as(as = "HexOrDecimalU256")]
+    pub buy_amount: U256,
+    #[serde_as(as = "HexOrDecimalU256")]
+    pub fee: U256,
+    pub solver: H160,
+}
+
+impl Quote {
+    fn from_domain(quote: domain::Quote) -> Self {
+        Quote {
+            sell_amount: quote.sell_amount.0,
+            buy_amount: quote.buy_amount.0,
+            fee: quote.fee.0,
+            solver: quote.solver.0,
+        }
+    }
+
+    pub fn to_domain(&self, order_uid: OrderUid) -> domain::Quote {
+        domain::Quote {
+            order_uid,
+            sell_amount: self.sell_amount.into(),
+            buy_amount: self.buy_amount.into(),
+            fee: self.fee.into(),
+            solver: self.solver.into(),
+        }
+    }
+}
+
 impl From<domain::auction::order::Side> for database::orders::OrderKind {
     fn from(side: domain::auction::order::Side) -> Self {
         match side {

diff --git a/crates/autopilot/src/infra/shadow/orderbook/mod.rs b/crates/autopilot/src/infra/shadow/orderbook/mod.rs
@@ -25,7 +25,7 @@ impl Orderbook {
             .error_for_status()?
             .json::<dto::Auction>()
             .await
-            .map(TryInto::try_into)
+            .map(dto::Auction::try_into_domain)
             .map_err(Into::<anyhow::Error>::into)?
     }
 }
diff --git a/crates/driver/src/infra/solver/dto/auction.rs b/crates/driver/src/infra/solver/dto/auction.rs
@@ -155,7 +155,7 @@ impl Auction {
                                 .protocol_fees
                                 .iter()
                                 .cloned()
-                                .map(Into::into)
+                                .map(FeePolicy::from_domain)
                                 .collect(),
                         ),
                         app_data: AppDataHash(order.app_data.0.into()),
@@ -407,8 +407,8 @@ pub enum FeePolicy {
     Volume { factor: f64 },
 }
 
-impl From<fees::FeePolicy> for FeePolicy {
-    fn from(value: order::FeePolicy) -> Self {
+impl FeePolicy {
+    pub fn from_domain(value: fees::FeePolicy) -> FeePolicy {
         match value {
             order::FeePolicy::Surplus {
                 factor,