From e1b06f02f3cee380b3493d9c225a07bebf4c0ade Mon Sep 17 00:00:00 2001 From: Tommy Verrall Date: Mon, 20 Oct 2025 11:39:50 +0200 Subject: [PATCH] Add optional builder pattern for BuilderConfig (non-breaking) Addresses @jstuczyn's feedback about too many arguments by adding BuilderConfigBuilder as an alternative to the existing new() method. --- nym-registration-client/src/builder/config.rs | 171 ++++++++++++++++++ 1 file changed, 171 insertions(+) diff --git a/nym-registration-client/src/builder/config.rs b/nym-registration-client/src/builder/config.rs index 47f5f7c8e8..f19e981f5a 100644 --- a/nym-registration-client/src/builder/config.rs +++ b/nym-registration-client/src/builder/config.rs @@ -56,6 +56,9 @@ pub struct MixnetClientConfig { } impl BuilderConfig { + /// Creates a new BuilderConfig with all required parameters. + /// + /// However, consider using `BuilderConfig::builder()` instead. #[allow(clippy::too_many_arguments)] pub fn new( entry_node: NymNodeWithKeys, @@ -84,6 +87,22 @@ impl BuilderConfig { } } + /// Creates a builder for BuilderConfig + /// + /// This is the preferred way to construct a BuilderConfig. + /// + /// # Example + /// ```ignore + /// let config = BuilderConfig::builder() + /// .entry_node(entry) + /// .exit_node(exit) + /// .user_agent(agent) + /// .build()?; + /// ``` + pub fn builder() -> BuilderConfigBuilder { + BuilderConfigBuilder::default() + } + pub fn mixnet_client_debug_config(&self) -> DebugConfig { if self.two_hops { two_hop_debug_config(&self.mixnet_client_config) @@ -235,6 +254,113 @@ fn true_to_disabled(val: bool) -> &'static str { if val { "disabled" } else { "enabled" } } +/// Builder for `BuilderConfig` +/// +/// This provides a more convinient way to construct a `BuilderConfig` compared to the +/// `new()` constructor with many arguments. +#[derive(Default)] +pub struct BuilderConfigBuilder { + entry_node: Option, + exit_node: Option, + data_path: Option, + mixnet_client_config: Option, + two_hops: bool, + user_agent: Option, + custom_topology_provider: Option>, + network_env: Option, + cancel_token: Option, + #[cfg(unix)] + connection_fd_callback: Option>, +} + +impl BuilderConfigBuilder { + pub fn new() -> Self { + Self::default() + } + + pub fn entry_node(mut self, entry_node: NymNodeWithKeys) -> Self { + self.entry_node = Some(entry_node); + self + } + + pub fn exit_node(mut self, exit_node: NymNodeWithKeys) -> Self { + self.exit_node = Some(exit_node); + self + } + + pub fn data_path(mut self, data_path: Option) -> Self { + self.data_path = data_path; + self + } + + pub fn mixnet_client_config(mut self, mixnet_client_config: MixnetClientConfig) -> Self { + self.mixnet_client_config = Some(mixnet_client_config); + self + } + + pub fn two_hops(mut self, two_hops: bool) -> Self { + self.two_hops = two_hops; + self + } + + pub fn user_agent(mut self, user_agent: UserAgent) -> Self { + self.user_agent = Some(user_agent); + self + } + + pub fn custom_topology_provider( + mut self, + custom_topology_provider: Box, + ) -> Self { + self.custom_topology_provider = Some(custom_topology_provider); + self + } + + pub fn network_env(mut self, network_env: NymNetworkDetails) -> Self { + self.network_env = Some(network_env); + self + } + + pub fn cancel_token(mut self, cancel_token: CancellationToken) -> Self { + self.cancel_token = Some(cancel_token); + self + } + + #[cfg(unix)] + pub fn connection_fd_callback( + mut self, + connection_fd_callback: Arc, + ) -> Self { + self.connection_fd_callback = Some(connection_fd_callback); + self + } + + /// Builds the `BuilderConfig`. + /// + /// Returns an error if any required field is missing. + pub fn build(self) -> Result { + Ok(BuilderConfig { + entry_node: self.entry_node.ok_or("entry_node is required")?, + exit_node: self.exit_node.ok_or("exit_node is required")?, + data_path: self.data_path, + mixnet_client_config: self + .mixnet_client_config + .ok_or("mixnet_client_config is required")?, + two_hops: self.two_hops, + user_agent: self.user_agent.ok_or("user_agent is required")?, + custom_topology_provider: self + .custom_topology_provider + .ok_or("custom_topology_provider is required")?, + network_env: self.network_env.ok_or("network_env is required")?, + cancel_token: self.cancel_token.ok_or("cancel_token is required")?, + #[cfg(unix)] + connection_fd_callback: self + .connection_fd_callback + .ok_or("connection_fd_callback is required")?, + }) + } +} + #[cfg(test)] mod tests { use super::*; @@ -247,4 +373,49 @@ mod tests { assert_eq!(config.min_mixnode_performance, None); assert_eq!(config.min_gateway_performance, None); } + + #[test] + fn test_builder_config_builder_fails_without_required_fields() { + // Building without any fields should fail + let result = BuilderConfig::builder().build(); + assert!(result.is_err()); + if let Err(err) = result { + assert!(err.contains("entry_node is required")); + } + } + + #[test] + fn test_builder_config_builder_fails_missing_individual_fields() { + // Test that each required field is validated + let result = BuilderConfig::builder().build(); + assert!(result.is_err()); + + // We can only test the first error message since build() short-circuits + if let Err(err) = result { + assert!( + err.contains("entry_node") + || err.contains("exit_node") + || err.contains("mixnet_client_config") + || err.contains("user_agent") + || err.contains("custom_topology_provider") + || err.contains("network_env") + || err.contains("cancel_token") + ); + } + } + + #[test] + fn test_builder_config_builder_method_chaining() { + // Test that builder methods chain properly and return Self + let builder = BuilderConfig::builder(); + + // Verify the builder returns itself for chaining + let builder = builder.two_hops(true); + let builder = builder.two_hops(false); + let builder = builder.data_path(None); + + // Builder should still fail because required fields are missing + let result = builder.build(); + assert!(result.is_err()); + } }