From b7d57ea83ebe1cebb56389fb540fc7c5207d8cb3 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 23 Feb 2022 18:31:41 +0000 Subject: [PATCH] Use &mut self in invoice updaters, not take-self-return-Self The take-self-return-Self idiom in Rust is substantially less usable than it is in Java, where its more common. Because we have to take self by move, it prevents using the update methods to actually update features, something we occasionally want to do. See, eg, the change in lightning-invoice where we previously had to copy and re-create an entire vec of fields just to update the features field, which is nuts. There are a few places where this makes things a little less clean, but the tradeoff to enable more effecient and broader uses of the update methods seems worth it. --- lightning-invoice/src/lib.rs | 18 ++++++++---------- lightning/src/ln/features.rs | 28 ++++++++++++++++------------ lightning/src/routing/router.rs | 3 ++- 3 files changed, 26 insertions(+), 23 deletions(-) diff --git a/lightning-invoice/src/lib.rs b/lightning-invoice/src/lib.rs index d676f6c28..dddc0d734 100644 --- a/lightning-invoice/src/lib.rs +++ b/lightning-invoice/src/lib.rs @@ -610,9 +610,9 @@ impl InvoiceBuilder InvoiceBuilder { /// Sets the payment secret and relevant features. pub fn payment_secret(mut self, payment_secret: PaymentSecret) -> InvoiceBuilder { - let features = InvoiceFeatures::empty() - .set_variable_length_onion_required() - .set_payment_secret_required(); + let mut features = InvoiceFeatures::empty(); + features.set_variable_length_onion_required(); + features.set_payment_secret_required(); self.tagged_fields.push(TaggedField::PaymentSecret(payment_secret)); self.tagged_fields.push(TaggedField::Features(features)); self.set_flags() @@ -622,13 +622,11 @@ impl InvoiceBuilder InvoiceBuilder { /// Sets the `basic_mpp` feature as optional. pub fn basic_mpp(mut self) -> Self { - self.tagged_fields = self.tagged_fields - .drain(..) - .map(|field| match field { - TaggedField::Features(f) => TaggedField::Features(f.set_basic_mpp_optional()), - _ => field, - }) - .collect(); + for field in self.tagged_fields.iter_mut() { + if let TaggedField::Features(f) = field { + f.set_basic_mpp_optional(); + } + } self } } diff --git a/lightning/src/ln/features.rs b/lightning/src/ln/features.rs index 0ccce88e9..7d7553dfa 100644 --- a/lightning/src/ln/features.rs +++ b/lightning/src/ln/features.rs @@ -318,15 +318,13 @@ mod sealed { impl Features { /// Set this feature as optional. - pub fn $optional_setter(mut self) -> Self { + pub fn $optional_setter(&mut self) { ::set_optional_bit(&mut self.flags); - self } /// Set this feature as required. - pub fn $required_setter(mut self) -> Self { + pub fn $required_setter(&mut self) { ::set_required_bit(&mut self.flags); - self } /// Checks if this feature is supported. @@ -506,7 +504,9 @@ impl InvoiceFeatures { /// [`PaymentParameters::for_keysend`]: crate::routing::router::PaymentParameters::for_keysend /// [`find_route`]: crate::routing::router::find_route pub(crate) fn for_keysend() -> InvoiceFeatures { - InvoiceFeatures::empty().set_variable_length_onion_optional() + let mut res = InvoiceFeatures::empty(); + res.set_variable_length_onion_optional(); + res } } @@ -838,11 +838,13 @@ mod tests { assert!(!features.requires_unknown_bits()); assert!(!features.supports_unknown_bits()); - let features = ChannelFeatures::empty().set_unknown_feature_required(); + let mut features = ChannelFeatures::empty(); + features.set_unknown_feature_required(); assert!(features.requires_unknown_bits()); assert!(features.supports_unknown_bits()); - let features = ChannelFeatures::empty().set_unknown_feature_optional(); + let mut features = ChannelFeatures::empty(); + features.set_unknown_feature_optional(); assert!(!features.requires_unknown_bits()); assert!(features.supports_unknown_bits()); } @@ -886,7 +888,8 @@ mod tests { fn convert_to_context_with_unknown_flags() { // Ensure the `from` context has fewer known feature bytes than the `to` context. assert!(InvoiceFeatures::known().flags.len() < NodeFeatures::known().flags.len()); - let invoice_features = InvoiceFeatures::known().set_unknown_feature_optional(); + let mut invoice_features = InvoiceFeatures::known(); + invoice_features.set_unknown_feature_optional(); assert!(invoice_features.supports_unknown_bits()); let node_features: NodeFeatures = invoice_features.to_context(); assert!(!node_features.supports_unknown_bits()); @@ -894,9 +897,9 @@ mod tests { #[test] fn set_feature_bits() { - let features = InvoiceFeatures::empty() - .set_basic_mpp_optional() - .set_payment_secret_required(); + let mut features = InvoiceFeatures::empty(); + features.set_basic_mpp_optional(); + features.set_payment_secret_required(); assert!(features.supports_basic_mpp()); assert!(!features.requires_basic_mpp()); assert!(features.requires_payment_secret()); @@ -938,7 +941,8 @@ mod tests { fn test_channel_type_mapping() { // If we map an InvoiceFeatures with StaticRemoteKey optional, it should map into a // required-StaticRemoteKey ChannelTypeFeatures. - let init_features = InitFeatures::empty().set_static_remote_key_optional(); + let mut init_features = InitFeatures::empty(); + init_features.set_static_remote_key_optional(); let converted_features = ChannelTypeFeatures::from_counterparty_init(&init_features); assert_eq!(converted_features, ChannelTypeFeatures::only_static_remote_key()); assert!(!converted_features.supports_any_optional_bits()); diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 65119f0f8..740fd691e 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -2369,7 +2369,8 @@ mod tests { let scorer = test_utils::TestScorer::with_penalty(0); // Disable nodes 1, 2, and 8 by requiring unknown feature bits - let unknown_features = NodeFeatures::known().set_unknown_feature_required(); + let mut unknown_features = NodeFeatures::known(); + unknown_features.set_unknown_feature_required(); add_or_update_node(&net_graph_msg_handler, &secp_ctx, &privkeys[0], unknown_features.clone(), 1); add_or_update_node(&net_graph_msg_handler, &secp_ctx, &privkeys[1], unknown_features.clone(), 1); add_or_update_node(&net_graph_msg_handler, &secp_ctx, &privkeys[7], unknown_features.clone(), 1); -- 2.39.5